Closed Bug 754452 Opened 12 years ago Closed 12 years ago

synthetic italics under GDI causes spacing problems for tahoma

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: al_9x, Assigned: jfkthame)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

noscript menu item, 3.5 vs. 15.0a1 (2012-05-11)
4.15 KB, image/png
Details
testcase, Arial font italic Arabic, font size 12
271 bytes, text/html
Details
Space in location bar with special font
62.78 KB, image/jpeg
Details
patch v3, decide whether to use GDI or Cairo fake-italic in gfxGDIFont::Initialize
16.02 KB, patch
jtd
: review+
Details | Diff | Splinter Review
patch transplanted to mozilla-beta
16.07 KB, patch
jfkthame
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
new profile, noscript menu

you can also see it in help -> about -> check for updates -> "firefox is up to date"
Blocks: 690917
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.
Not yet tested, but I think this is an approach that should help.
This bug occurs with Arial font italic Arabic, with the font size 12.

Screenshot: http://i449.photobucket.com/albums/qq217/movh/2a3e43a0.png
Attachment #623845 - Attachment mime type: text/plain → text/html
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 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 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.
Attachment #623706 - Flags: review?(jdaggett)
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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 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.
(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
(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).
(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.
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.
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
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
This is what made ​​me think that bug 724231 is a regression from bug 603879.
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.)
(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/
(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
jdaggett, review ping...?
can you get this into 14.0? it's a pretty annoying regression
(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.
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.
URL: http://people.mozilla.org/~jdaggett/t...
especially since it's in content as well, what's holding this up?
Summary: bad tahoma italic spacing in chrome (with or without harfbuzz) → bad tahoma italic spacing (with or without harfbuzz)
Severity: normal → major
Summary: bad tahoma italic spacing (with or without harfbuzz) → synthetic italics under GDI causes spacing problems for tahoma
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.
(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.
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.
Assignee: nobody → jfkthame
Attachment #623706 - Attachment is obsolete: true
Attachment #623706 - Flags: review?(jdaggett)
Attachment #632697 - Flags: review?(jdaggett)
(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.
(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.
Argh, spelling regression there, s/write/right/.
Blocks: 724231
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.
Attachment #632697 - Flags: review?(jdaggett) → review-
Here's a version along the lines suggested in comment #33.
Attachment #638000 - Flags: review?(jdaggett)
Attachment #638000 - Flags: review?(jdaggett) → review+
Attachment #632697 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b77545d6a4f5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Can we back port the patch and Bug 724231 patch to beta and aurora?
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.
(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.
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.
What about 15.0?
Patch transplanted to the m-b tree. Only one trivial fixup was needed to make it apply correctly; carrying over r=jdaggett.
Attachment #644640 - Flags: review+
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
Attachment #644640 - Flags: approval-mozilla-beta?
Keywords: regression
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.
Attachment #644640 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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
status-firefox15: fixed → verified
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
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: