Closed
Bug 754452
Opened 13 years ago
Closed 12 years ago
synthetic italics under GDI causes spacing problems for tahoma
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
VERIFIED
FIXED
mozilla16
People
(Reporter: al_9x, Assigned: jfkthame)
References
(Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(5 files, 2 obsolete files)
4.15 KB,
image/png
|
Details | |
271 bytes,
text/html
|
Details | |
62.78 KB,
image/jpeg
|
Details | |
16.02 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
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"
Assignee | ||
Comment 1•13 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.
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>
</p>
<p>
<span style="font-size:12px;"><em>الخط العربي في فايرفوكس</em></span></p>
</body></html>
Assignee | ||
Comment 6•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
This is what made me think that bug 724231 is a regression from bug 603879.
Assignee | ||
Comment 17•13 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•13 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•13 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
Comment 23•12 years ago
|
||
Assignee | ||
Comment 24•12 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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
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•12 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•12 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 33•12 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•12 years ago
|
||
Here's a version along the lines suggested in comment #33.
Attachment #638000 -
Flags: review?(jdaggett)
Updated•12 years ago
|
Attachment #638000 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 35•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77545d6a4f5
Target Milestone: --- → mozilla16
Assignee | ||
Updated•12 years ago
|
Attachment #632697 -
Attachment is obsolete: true
Comment 36•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b77545d6a4f5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 38•12 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•12 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•12 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.
Assignee | ||
Comment 42•12 years ago
|
||
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•12 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•12 years ago
|
Keywords: regression
Comment 44•12 years ago
|
||
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+
Updated•12 years ago
|
status-firefox15:
--- → affected
Updated•12 years ago
|
status-firefox16:
--- → fixed
Assignee | ||
Comment 45•12 years ago
|
||
Pushed to beta, marking as fixed for ff15: https://hg.mozilla.org/releases/mozilla-beta/rev/bbd4b02700f1
Comment 46•12 years ago
|
||
(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•12 years ago
|
||
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
Updated•12 years ago
|
QA Contact: paul.silaghi
You need to log in
before you can comment on or make changes to this bug.
Description
•