The default bug view has changed. See this FAQ.

synthetic italics under GDI causes spacing problems for tahoma

VERIFIED FIXED in Firefox 15

Status

()

Core
Layout: Text
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: al_9x, Assigned: jfkthame)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla16
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 verified, firefox16 verified)

Details

(URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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"
(Reporter)

Updated

5 years ago
Blocks: 690917
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

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

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

Updated

5 years ago
Attachment #623845 - Attachment mime type: text/plain → text/html

Comment 4

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

5 years ago
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>
(Assignee)

Comment 6

5 years ago
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)
(Assignee)

Comment 7

5 years ago
(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
(Assignee)

Comment 8

5 years ago
(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

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

5 years ago
(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
(Assignee)

Comment 11

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 13

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

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

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

5 years ago
This is what made ​​me think that bug 724231 is a regression from bug 603879.
(Assignee)

Comment 17

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

5 years ago
(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/
(Reporter)

Comment 19

5 years ago
(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
(Assignee)

Comment 20

5 years ago
jdaggett, review ping...?
(Reporter)

Comment 21

5 years ago
can you get this into 14.0? it's a pretty annoying regression
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762467

Comment 23

5 years ago
Created attachment 631006 [details]
Space in location bar with special font
(Assignee)

Comment 24

5 years ago
(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

5 years ago
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.
(Reporter)

Comment 26

5 years ago
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)

Updated

5 years ago
Severity: normal → major
Summary: bad tahoma italic spacing (with or without harfbuzz) → synthetic italics under GDI causes spacing problems for tahoma

Comment 27

5 years ago
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.
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
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.
Assignee: nobody → jfkthame
Attachment #623706 - Attachment is obsolete: true
Attachment #623706 - Flags: review?(jdaggett)
Attachment #632697 - Flags: review?(jdaggett)
(Assignee)

Comment 30

5 years ago
(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

5 years ago
(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

5 years ago
Argh, spelling regression there, s/write/right/.
(Assignee)

Updated

5 years ago
Blocks: 724231

Comment 33

5 years ago
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-
(Assignee)

Comment 34

5 years ago
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.
Attachment #638000 - Flags: review?(jdaggett)

Updated

5 years ago
Attachment #638000 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 35

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77545d6a4f5
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Attachment #632697 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b77545d6a4f5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 37

5 years ago
Can we back port the patch and Bug 724231 patch to beta and aurora?
(Assignee)

Comment 38

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

5 years ago
(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

5 years ago
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.
(Reporter)

Comment 41

5 years ago
What about 15.0?
(Assignee)

Comment 42

5 years ago
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.
Attachment #644640 - Flags: review+
(Assignee)

Comment 43

5 years ago
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?
(Assignee)

Updated

5 years ago
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+
status-firefox15: --- → affected
status-firefox16: --- → fixed
(Assignee)

Comment 45

5 years ago
Pushed to beta, marking as fixed for ff15:
https://hg.mozilla.org/releases/mozilla-beta/rev/bbd4b02700f1
status-firefox15: affected → fixed
(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.