Last Comment Bug 754452 - synthetic italics under GDI causes spacing problems for tahoma
: synthetic italics under GDI causes spacing problems for tahoma
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 Windows XP
: -- major with 2 votes (vote)
: mozilla16
Assigned To: Jonathan Kew (:jfkthame)
: Paul Silaghi, QA [:pauly]
Mentors:
http://people.mozilla.org/~jdaggett/t...
: 762467 (view as bug list)
Depends on:
Blocks: 690917 724231
  Show dependency treegraph
 
Reported: 2012-05-11 14:22 PDT by al_9x
Modified: 2012-08-30 05:46 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
noscript menu item, 3.5 vs. 15.0a1 (2012-05-11) (4.15 KB, image/png)
2012-05-11 14:22 PDT, al_9x
no flags Details
patch, try to use GDI synthetic-italic styling if this is safe (8.18 KB, patch)
2012-05-14 09:44 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
testcase, Arial font italic Arabic, font size 12 (271 bytes, text/html)
2012-05-14 15:23 PDT, blinky
no flags Details
Space in location bar with special font (62.78 KB, image/jpeg)
2012-06-07 09:12 PDT, Loic
no flags Details
patch v2, try to use GDI synthetic-italic styling if this is safe (14.95 KB, patch)
2012-06-13 08:02 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review-
Details | Diff | Splinter Review
patch v3, decide whether to use GDI or Cairo fake-italic in gfxGDIFont::Initialize (16.02 KB, patch)
2012-06-29 13:47 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
Details | Diff | Splinter Review
patch transplanted to mozilla-beta (16.07 KB, patch)
2012-07-21 07:59 PDT, Jonathan Kew (:jfkthame)
jfkthame: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description al_9x 2012-05-11 14:22:52 PDT
Created attachment 623300 [details]
noscript menu item, 3.5 vs. 15.0a1 (2012-05-11)

new profile, noscript menu

you can also see it in help -> about -> check for updates -> "firefox is up to date"
Comment 1 Jonathan Kew (:jfkthame) 2012-05-14 09:42:27 PDT
I assume this is a result of bug 724231, where we switched to using a cairo transform to create "synthetic" italics (slanting the regular font) when no real italic face is available. Unfortunately, the presence of a transform (other than simple scaling) disrupts the hinting that would normally be applied.

The switch to using the cairo transform was needed to avoid cases where asking GDI to synthesize "italics" may result in it actually using an incompatible real-italic face. However, I think we can detect whether there is actually any risk of that, and return to using GDI styling in most cases - particularly, for common examples such as this one.
Comment 2 Jonathan Kew (:jfkthame) 2012-05-14 09:44:15 PDT
Created attachment 623706 [details] [diff] [review]
patch, try to use GDI synthetic-italic styling if this is safe

Not yet tested, but I think this is an approach that should help.
Comment 3 blinky 2012-05-14 15:23:02 PDT
Created attachment 623845 [details]
testcase, Arial font italic Arabic, font size 12

This bug occurs with Arial font italic Arabic, with the font size 12.

Screenshot: http://i449.photobucket.com/albums/qq217/movh/2a3e43a0.png
Comment 4 blinky 2012-05-14 15:53:20 PDT
Italic font in search rectangle, appears not bold when you disable hardware acceleration in Firefox version Arabic.

Screenshot: http://i449.photobucket.com/albums/qq217/movh/e2b1def9.png
Comment 5 blinky 2012-05-14 16:31:05 PDT
Comment on attachment 623845 [details]
testcase, Arial font italic Arabic, font size 12

<html><head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
		<title>Preview</title>
	</head>
	<body>
		<p>
			&nbsp;</p>
		<p>
			<span style="font-size:12px;"><em>الخط العربي في فايرفوكس</em></span></p>

</body></html>
Comment 6 Jonathan Kew (:jfkthame) 2012-05-14 23:59:09 PDT
Comment on attachment 623706 [details] [diff] [review]
patch, try to use GDI synthetic-italic styling if this is safe

Checked that this improves the appearance of synthetic-italic examples such as Tahoma under GDI on Win7; should help similarly on XP.

Tryserver build available at https://tbpl.mozilla.org/?tree=Try&rev=f6c9dd97781f.
Comment 7 Jonathan Kew (:jfkthame) 2012-05-15 00:00:48 PDT
(In reply to TB from comment #4)
> Italic font in search rectangle, appears not bold when you disable hardware
> acceleration in Firefox version Arabic.

I don't think that's related to this issue; please file a separate report about it.
Comment 8 Jonathan Kew (:jfkthame) 2012-05-15 00:05:18 PDT
(In reply to TB from comment #3)
> Created attachment 623845 [details]
> testcase, Arial font italic Arabic, font size 12
> 
> This bug occurs with Arial font italic Arabic, with the font size 12.
> 
> Screenshot: http://i449.photobucket.com/albums/qq217/movh/2a3e43a0.png

The patch here will not fix the issue for Arabic text using Arial (or Times) with italic style, because those are precisely the cases that are affected by bug 724231 and forced us to switch to using a transform to slant the text. To solve the poor spacing that can occur there, we'd need to figure out how to maintain proper hinting and grid-fitting of the glyphs, despite the presence of the transform, which is a trickier issue. Fortunately, once this patch is applied, the issue will only affect a few relatively rare situations.
Comment 9 Ekanan Ketunuti 2012-05-15 00:25:23 PDT
Comment on attachment 623706 [details] [diff] [review]
patch, try to use GDI synthetic-italic styling if this is safe

looks good. this patch also fixes bug 728537.
Comment 10 blinky 2012-05-15 01:59:18 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> (In reply to TB from comment #4)
> > Italic font in search rectangle, appears not bold when you disable hardware
> > acceleration in Firefox version Arabic.
> 
> I don't think that's related to this issue; please file a separate report
> about it.

I created a new report in Bug 755205

Thanks
Comment 11 Jonathan Kew (:jfkthame) 2012-05-15 03:05:31 PDT
(In reply to Ekanan Ketunuti from comment #9)
> Comment on attachment 623706 [details] [diff] [review]
> patch, try to use GDI synthetic-italic styling if this is safe
> 
> looks good. this patch also fixes bug 728537.

More precisely, it will fix it for the same set of cases as it fixes the spacing issue (i.e. it'll resolve the great majority of use cases, but there will be a few exceptions such as "italic" Arabic text in Times New Roman or Arial fonts, and probably a few others).
Comment 12 blinky 2012-05-17 06:00:17 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> because those are precisely the cases that are affected by bug 724231 and forced us to switch to using a 
> transform to slant the text.

I suspect the bug 724231 is a regression from bug 603879.
Comment 13 Jonathan Kew (:jfkthame) 2012-05-17 07:26:35 PDT
No, it's not related to bug 603879. The main cause of bug 724231 was the fix in bug 668813, because it causes us to use synthetic styling instead of falling back to a different font family in certain cases.
Comment 14 blinky 2012-05-17 09:26:14 PDT
Based on the testcase in bug 724231, The text appears distorted since the release 4.0b8pre (2010-11-21)

See the screenshot: http://i449.photobucket.com/albums/qq217/movh/158333d6.png

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101121 Firefox/4.0b8pre

http://hg.mozilla.org/mozilla-central/rev/be4b69a4babf

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-21-04-mozilla-central/firefox-4.0b8pre.en-US.win32.zip
Comment 15 blinky 2012-05-17 09:35:39 PDT
The text appears correctly in version 4.0b8pre (2010-11-20)

Screenshot: http://i449.photobucket.com/albums/qq217/movh/9ebd7a90.png

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101120 Firefox/4.0b8pre

http://hg.mozilla.org/mozilla-central/rev/baa6cc2f72e4

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-20-04-mozilla-central/firefox-4.0b8pre.en-US.win32.zip
Comment 16 blinky 2012-05-17 09:43:14 PDT
This is what made ​​me think that bug 724231 is a regression from bug 603879.
Comment 17 Jonathan Kew (:jfkthame) 2012-05-17 09:45:32 PDT
The failure in the Greek example appears on that date because we enabled harfbuzz shaping for Greek script, but in principle it could have failed in other cases even prior to that. They were just rarer; whereas bug 668813 made them show up in a much more common case (italicized Arabic). But the fundamental problem already existed.

(In any case, that's not really very interesting at this point, as the failure is understood and fixed in current code.)
Comment 18 blinky 2012-05-17 11:10:09 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> The switch to using the cairo transform was needed to avoid cases where
> asking GDI to synthesize "italics" may result in it actually using an
> incompatible real-italic face.

Did you mean Cairo software library http://www.cairographics.org?
If Yes, Updating to the latest version may solve the bug.

latest version here http://www.cairographics.org/news/cairo-1.12.2/
Comment 19 al_9x 2012-05-18 13:06:35 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Comment on attachment 623706 [details] [diff] [review]
> patch, try to use GDI synthetic-italic styling if this is safe
> 
> Checked that this improves the appearance of synthetic-italic examples such
> as Tahoma under GDI on Win7; should help similarly on XP.
> 

it does, thanks
Comment 20 Jonathan Kew (:jfkthame) 2012-05-25 03:30:22 PDT
jdaggett, review ping...?
Comment 21 al_9x 2012-06-05 16:00:46 PDT
can you get this into 14.0? it's a pretty annoying regression
Comment 22 Jonathan Kew (:jfkthame) 2012-06-07 08:51:19 PDT
*** Bug 762467 has been marked as a duplicate of this bug. ***
Comment 23 Loic 2012-06-07 09:12:17 PDT
Created attachment 631006 [details]
Space in location bar with special font
Comment 24 Jonathan Kew (:jfkthame) 2012-06-07 09:24:01 PDT
(In reply to Loic from comment #23)
> Created attachment 631006 [details]
> Space in location bar with special font

I don't see how that's related to this bug report. The screenshot looks like an example of bug 598900.
Comment 25 John Daggett (:jtd) 2012-06-12 00:55:50 PDT
Steps to reproduce:

Run FF13 or Nightly on XP or Win7 with 'gfx.direct2d.disabled' set to true.

1. Open test URL

Result: the three text boxes at the top of the page show italic Tahoma at 13px with very poor spacing (e.g. the bold 'new er' on the second line)

Expected result: first two text boxes should appear with more even spacing, as in Chrome/IE9.
Comment 26 al_9x 2012-06-12 15:03:58 PDT
especially since it's in content as well, what's holding this up?
Comment 27 John Daggett (:jtd) 2012-06-13 00:58:29 PDT
We've fiddled with this code a number of times and I'd like to try and
get it right this time, the GDI handling of synthetic italics should
match other platforms (including DirectWrite) and minimize the spacing
problems as much as possible.

The testcase URL shows two problems with the patch, the 'src:
local(Tahoma)' case still shows the spacing problem and declaring a
regular face to be italic shouldn't result in synthetic oblique (the
green case).  Other platforms (including DirectWrite) render these
correctly.  These are both regressions caused by the original 724231 patch.

We should only be using the cairo oblique path when (1) it's a local user font and (2) the family has italic faces.  This patch appears still applies cairo oblique in the local(Tahoma) case which it really shouldn't.
Comment 28 Jonathan Kew (:jfkthame) 2012-06-13 07:58:00 PDT
(In reply to John Daggett (:jtd) (Away 15-22 June) from comment #27)

> We should only be using the cairo oblique path when (1) it's a local user
> font and (2) the family has italic faces.

Or in the original problem case: when it's an installed platform font that has an italic face, but font-matching has chosen a non-italic face because of character coverage.

>  This patch appears still applies
> cairo oblique in the local(Tahoma) case which it really shouldn't.

For a local user font, we don't have details of other members of the (platform, not CSS) family readily available, so we'd have to do additional work to determine whether there's an italic face that GDI might automagically use.

I suppose we could make gfxGDIFontList::LookupLocalFont record a reference to the platform family where the font was found in the new "local" fontEntry it creates, so that we'd be able to check on the other platform family members.
Comment 29 Jonathan Kew (:jfkthame) 2012-06-13 08:02:19 PDT
Created attachment 632697 [details] [diff] [review]
patch v2, try to use GDI synthetic-italic styling if this is safe

Here's a version which handles the local-tahoma case, as suggested above, by keeping track of the "platform family" associated with a src:local() face.
Comment 30 Jonathan Kew (:jfkthame) 2012-06-13 08:04:09 PDT
(In reply to John Daggett (:jtd) (Away 15-22 June) from comment #27)

> declaring a
> regular face to be italic shouldn't result in synthetic oblique (the
> green case

This should be a separate bug, IMO, and needn't block fixing the spacing issue here.
Comment 31 John Daggett (:jtd) 2012-06-13 17:03:43 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> (In reply to John Daggett (:jtd) (Away 15-22 June) from comment #27)
> 
> > declaring a
> > regular face to be italic shouldn't result in synthetic oblique (the
> > green case
> 
> This should be a separate bug, IMO, and needn't block fixing the spacing
> issue here.

It's a regression from bug 724231.  I think we should get this write and stop futzing about with a sequence of tweaks that cause regressions in other places.  Normally I would agree but in this case I think focusing on a solution that covers the edge cases is important.
Comment 32 John Daggett (:jtd) 2012-06-13 17:04:42 PDT
Argh, spelling regression there, s/write/right/.
Comment 33 John Daggett (:jtd) 2012-06-26 23:01:13 PDT
Comment on attachment 632697 [details] [diff] [review]
patch v2, try to use GDI synthetic-italic styling if this is safe

> -    FillLogFont(logFont, mAdjustedSize);
> +    fakeItalicApplied = FillLogFont(logFont, mAdjustedSize);

Rather than making the GDI vs. cairo synthetics decision down in
FillLogFont, determine that within gfxGDIFont::Initialize and pass a
'setGDIItalic' flag into FillLogFont. That way both the italic and
bold decision is made in roughly the same area of code.

>      // create a font entry for a font with a given name
>      static GDIFontEntry* CreateFontEntry(const nsAString& aName,
>                                           gfxWindowsFontType aFontType,
>                                           bool aItalic,
>                                           PRUint16 aWeight, PRInt16 aStretch,
> -                                         gfxUserFontData* aUserFontData);
> +                                         gfxUserFontData* aUserFontData,
> +                                         gfxFontFamily* aPlatformFamily);

This is only needed to determine whether the existing family has an
italic face or not. But that can just a simply be determined within
gfxGDIFontList::LookupLocalFont and then passed as a flag, so that a
simple bool flag is stored rather than a pointer.
Comment 34 Jonathan Kew (:jfkthame) 2012-06-29 13:47:25 PDT
Created attachment 638000 [details] [diff] [review]
patch v3, decide whether to use GDI or Cairo fake-italic in gfxGDIFont::Initialize

Here's a version along the lines suggested in comment #33.
Comment 36 Ryan VanderMeulen [:RyanVM] 2012-07-03 16:08:27 PDT
https://hg.mozilla.org/mozilla-central/rev/b77545d6a4f5
Comment 37 xunxun 2012-07-04 03:01:28 PDT
Can we back port the patch and Bug 724231 patch to beta and aurora?
Comment 38 Jonathan Kew (:jfkthame) 2012-07-04 04:23:26 PDT
Bug 724231 should already be there - it landed for mozilla-13, I believe.

Or did you mean bug 764805 (the other issue mentioned here as a regression from 724231)? I don't think that is serious enough to justify backporting - it's a minor issue in an obscure and unusual use-case that other browsers don't handle consistently either.

I think we could consider backporting this fix, but let's give it a few days on Nightly first to see if any problems show up.
Comment 39 xunxun 2012-07-04 04:41:01 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #38)
> Bug 724231 should already be there - it landed for mozilla-13, I believe.
> 
> Or did you mean bug 764805 (the other issue mentioned here as a regression
> from 724231)? I don't think that is serious enough to justify backporting -
> it's a minor issue in an obscure and unusual use-case that other browsers
> don't handle consistently either.
> 
> I think we could consider backporting this fix, but let's give it a few days
> on Nightly first to see if any problems show up.

Yea. I mean bug 764805 patch.

Waiting for the next release fix.
Comment 40 John Daggett (:jtd) 2012-07-04 17:35:59 PDT
I agree, this fix needs to go to beta/aurora to fix the regression caused by bug 724231 but the fix for bug 764805 doesn't need to be rushed.
Comment 41 al_9x 2012-07-19 20:33:42 PDT
What about 15.0?
Comment 42 Jonathan Kew (:jfkthame) 2012-07-21 07:59:55 PDT
Created attachment 644640 [details] [diff] [review]
patch transplanted to mozilla-beta

Patch transplanted to the m-b tree. Only one trivial fixup was needed to make it apply correctly; carrying over r=jdaggett.
Comment 43 Jonathan Kew (:jfkthame) 2012-07-21 08:10:29 PDT
Comment on attachment 644640 [details] [diff] [review]
patch transplanted to mozilla-beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 724231

User impact if declined: poor rendering (erratic glyph spacing) in italic text with some very common fonts on Windows/GDI systems.

Testing completed (on m-c, etc.): several weeks on m-c with no observed problems; users confirm the issue is fixed there.

Risk to taking this patch (and alternatives if risky): minimal risk, essentially reverts to pre-724231 behavior for most fonts, making that fix more narrowly targeted to the cases where a real problem existed. The alternative of backing out 724231 is known to cause more serious breakage (completely garbled text, though in less-common cases).

This went to Aurora in the recent uplift, so the fix will go out in FF16, but given the low risk and the visible nature of the regression, I think we should consider uplifting it to FF15.

String or UUID changes made by this patch: none
Comment 44 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-23 12:55:19 PDT
Comment on attachment 644640 [details] [diff] [review]
patch transplanted to mozilla-beta

Thanks for the risk eval, looks like taking this on 15 isn't too risky - if you can land today and get this into the Beta that goes to build tomorrow, great.
Comment 45 Jonathan Kew (:jfkthame) 2012-07-23 14:42:21 PDT
Pushed to beta, marking as fixed for ff15:
https://hg.mozilla.org/releases/mozilla-beta/rev/bbd4b02700f1
Comment 46 Paul Silaghi, QA [:pauly] 2012-08-13 07:06:25 PDT
(In reply to John Daggett (:jtd) from comment #25)
> Steps to reproduce:
> 
> Run FF13 or Nightly on XP or Win7 with 'gfx.direct2d.disabled' set to true.
> 
> 1. Open test URL
> 
> Result: the three text boxes at the top of the page show italic Tahoma at
> 13px with very poor spacing (e.g. the bold 'new er' on the second line)
> 
> Expected result: first two text boxes should appear with more even spacing,
> as in Chrome/IE9.

Verified fixed on Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0b4
Comment 47 Paul Silaghi, QA [:pauly] 2012-08-30 04:55:13 PDT
Saw the issue on FF 13.0.1.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0b1

Note You need to log in before you can comment on or make changes to this bug.