Times New Roman Italic format error

RESOLVED FIXED

Status

()

Core
Layout: Text
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Gary, Assigned: jfkthame)

Tracking

(Depends on: 1 bug, {regression, relnote})

unspecified
regression, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5 fixed, blocking2.0 -)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 ( .NET CLR 3.5.30729; .NET4.0C)
Build Identifier: 3.1.2

Text formats oddly when using quotes with Times Roman Italic.
EXAMPLE

When composing text like this In Times Roman Italic, "real example" is corrupted.

After reformatting back to non-italic, the error goes away.


Reproducible: Always

Steps to Reproduce:
1. Start writing a new message in Thunderbird
2. type "real example"
3.Format times Roman Italic
Actual Results:  
Quote characters cause garbled text when Times Roman Italic is used
Can we get a screenshot ?
Created attachment 526563 [details]
testcase
Created attachment 526564 [details]
What I see with the testcase
Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110416 Firefox/6.0a1 ID:20110416030532

I can confirm this on Firefox/Nightly, so this should probably be a core bug.

See the attached testcase and results. Same results with hardware acceleration on/off. Nvidia GeForce 9500M GS, quite recent drivers (updated two-three months ago).
cc'ing jdaggett as I noticed you in a few similar bugs. Or at least you will know the right person to cc :)

Comment 6

6 years ago
Confirmed on
http://hg.mozilla.org/mozilla-central/rev/b140e7746652
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110416 Firefox/6.0a1 ID:20110416030532

First regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/052a1c415457
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100314 Minefield/3.7a3pre ID:20100314214215
Broken:
http://hg.mozilla.org/mozilla-central/rev/8ff6056308bd
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100315 Minefield/3.7a3pre ID:20100315042737
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=052a1c415457&tochange=8ff6056308bd
Triggered by:
8ff6056308bd	Jonathan Kew — bug 502906 - part 3 - factor out Uniscribe and GDI shapers from Windows GDI font code. r=jdaggett


Fixed window(cached m-c hourly):
Fails:
http://hg.mozilla.org/mozilla-central/rev/cc06207827f4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre ID:20100902020913
Works again:
http://hg.mozilla.org/mozilla-central/rev/dc2939f2640d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre ID:20100902051352
Fixed pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc06207827f4&tochange=dc2939f2640d


Second Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/b97a060746f9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre ID:20110324073329
Broken again:
http://hg.mozilla.org/mozilla-central/rev/bf15f5d6cf32
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre ID:20110324081839
pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b97a060746f9&tochange=bf15f5d6cf32
Triggered by:
Bug 635639 - Vowels are not rendered correctly in some Persian/Arabic/Hebrew fonts
Blocks: 635639
Severity: trivial → major
Status: UNCONFIRMED → NEW
Component: Message Compose Window → Layout: Text
Ever confirmed: true
Keywords: regression
Product: Thunderbird → Core
QA Contact: message-compose → layout.fonts-and-text
Version: unspecified → Trunk

Updated

6 years ago
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
Version: Trunk → 2.0 Branch

Comment 7

6 years ago
And this bug happens on on Firefox 4.0.1 and 4.0.2pre.
http://hg.mozilla.org/releases/mozilla-2.0/rev/fca718600ca0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 ID:20110413222027
http://hg.mozilla.org/releases/mozilla-2.0/rev/b69c49683122
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.2pre) Gecko/20110415 Firefox/4.0.2pre ID:20110415030208
blocking2.0: --- → ?
(Assignee)

Comment 8

6 years ago
This is actually a font bug (an error in the Times New Roman Italic font shipped with Windows); see discussion in bug 573407. When using harfbuzz for text shaping, we used to ignore the GDEF table, and so the problem didn't show up. This was changed in bug 635639, in order to render diacritics properly, but the side-effect is that we now treat the double-quote in Times Italic as a diacritic too - because that's what the font claims.

Attachment 452667 [details] shows the same behavior in Notepad, illustrating that it is a Windows font bug and not just a Gecko issue.

Comment 9

6 years ago
(In reply to comment #8)
> This was changed in bug 635639, in order to render diacritics properly, but
> the side-effect is that we now treat the double-quote in Times Italic as a
> diacritic too - because that's what the font claims.
> 
> Attachment 452667 [details] shows the same behavior in Notepad, illustrating that it is a
> Windows font bug and not just a Gecko issue.

However, data:text/html,<i>"Lorem</i> shows properly on
http://hg.mozilla.org/mozilla-central/rev/b140e7746652
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110416 Firefox/6.0a1 ID:20110416030532
(Assignee)

Comment 10

6 years ago
So it does. But
  data:text/html,<i>"Lorem" "ipsum"</i>
shows that there are indeed problems with the double-quote in Times Italic - the bug manifests slightly differently under harfbuzz than it does under uniscribe or directwrite, but it's still a font error.
(Assignee)

Comment 11

6 years ago
Created attachment 526622 [details] [diff] [review]
patch, ignore the GDEF table in Times New Roman [Bold]Italic

I really don't like putting a hack like this in the code, but I don't see how else we can resolve this (i.e. work around the buggy font) while keeping the diacritic fix we need for bug 635639. So here goes.... yuck.

The idea here is simply to make harfbuzz ignore the GDEF table in the Times New Roman Italic and BoldItalic fonts on Windows, which are the ones exhibiting this bug. Fortunately, TNR doesn't include Arabic in the Italic faces, so this won't regress Arabic vowel rendering. And also fortunately, the Hebrew diacritics in these faces don't appear to suffer from the diacritic-spacing problem that arose in bug 635639. So this seems to be reasonably safe, and it's narrowly targeted to just the two known problem faces.
Assignee: nobody → jfkthame
Attachment #526622 - Flags: review?(jdaggett)
(Assignee)

Comment 12

6 years ago
Hmm, it looks like OS X also ships a problematic version of Times New Roman (it's in /Library/Fonts on my 10.6 system), so if we're going to do a hack like this, we should probably do it for the Mac font backend too. The issue won't be as widespread there because Times New Roman is not a common default font (note that it's distinct from Times Roman, which does _not_ have the bug), but it will show up if a page explicitly specifies TNR as its font-family.
(Assignee)

Comment 13

6 years ago
Created attachment 526633 [details] [diff] [review]
patch, v2 - move the anti-Times-New-Roman-Italic-GDEF hack to gfxHarfBuzzShaper, so it applies to all platforms

This fixes the problem across all platforms, rather than only on Windows (although in practice that's the main place it will show up).
Attachment #526622 - Attachment is obsolete: true
Attachment #526633 - Flags: review?(jdaggett)
Attachment #526622 - Flags: review?(jdaggett)

Comment 14

6 years ago
Created attachment 526650 [details]
revised testcase, include text-rendering

By including 'text-rendering: optimizeLegibility;', the same problem appears in Chrome, which uses Uniscribe with that property setting.  With Harfbuzz disabled, same problem appears with the DirectWrite shaping code.

Comment 15

6 years ago
Comment on attachment 526633 [details] [diff] [review]
patch, v2 - move the anti-Times-New-Roman-Italic-GDEF hack to gfxHarfBuzzShaper, so it applies to all platforms

+    // bug 589682 - ignore the GDEF table in Times Italic, because it's buggy
+    if (aTag == TRUETYPE_TAG('G','D','E','F') &&
+        !font->GetFontEntry()->IsUserFont() &&
+        (font->GetStyle()->style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) &&
+        font->GetFontEntry()->FamilyName().EqualsLiteral("Times New Roman"))
+    {
+        return nsnull;
+    }

*sigh*

If we really must do this I'd prefer this to be a mIgnoreGDEF bit flag in the gfxFontEntry (there are already several so this won't affect space requirements), so that the code above reduces to if (font->GetFontEntry()->IgnoreGDEF()) { .... }.

We should definitely pass this along to Simon at MS.
(Assignee)

Comment 16

6 years ago
Created attachment 526665 [details] [diff] [review]
patch, v3 - use a bit flag in the font entry

This uses a flag in the font entry to mark the cases where we want to ignore the GDEF table. It's set by gfxFontFamily::AddFontEntry when adding faces to the family, as that avoids duplicating the logic in the multiple font-list implementations that originally create the entries.
Attachment #526633 - Attachment is obsolete: true
Attachment #526665 - Flags: review?(jdaggett)
Attachment #526633 - Flags: review?(jdaggett)

Updated

6 years ago
Attachment #526665 - Flags: review?(jdaggett) → review+
So the original bug was reported on 1.9.2 branch, not on 2.0 branch, but its unclear to me if bug 502906 or bug 635639 actually made it onto that branch.
Version: 2.0 Branch → 1.9.2 Branch
(Assignee)

Comment 18

6 years ago
(In reply to comment #17)
> So the original bug was reported on 1.9.2 branch, not on 2.0 branch, but its
> unclear to me if bug 502906 or bug 635639 actually made it onto that branch.

No, none of that is on 1.9.2. (It's actually bug 449292 that "fixed" - or at least avoided - this for 2.x, before 635639 brought it back.)

A fix for 1.9.2 would be messier, as we aren't using harfbuzz and so the strategy of just blocking reading of the bad GDEF table won't help; we'd have to arrange to bypass or override Uniscribe's layout with these fonts. Personally, I'm not sure it's worth the complexity and risk on that branch.
(Assignee)

Comment 19

6 years ago
Created attachment 526680 [details] [diff] [review]
reftest to verify that double-quote in times italic has non-zero width

Test designed to alert us if we inadvertently break or circumvent this hack sometime.
Attachment #526680 - Flags: review?(jdaggett)
(In reply to comment #18)
> (In reply to comment #17)
> > So the original bug was reported on 1.9.2 branch, not on 2.0 branch, but its
> > unclear to me if bug 502906 or bug 635639 actually made it onto that branch.
> 
> No, none of that is on 1.9.2. (It's actually bug 449292 that "fixed" - or at
> least avoided - this for 2.x, before 635639 brought it back.)
> 
> A fix for 1.9.2 would be messier, as we aren't using harfbuzz and so the
> strategy of just blocking reading of the bad GDEF table won't help; we'd have
> to arrange to bypass or override Uniscribe's layout with these fonts.
> Personally, I'm not sure it's worth the complexity and risk on that branch.

Ok, thanks, I just wanted to make sure we hadn't missed the fact that this was in the 1.9.2 branch (which could have been a different bug of sorts).
(In reply to comment #20)
>
> Ok, thanks, I just wanted to make sure we hadn't missed the fact that this was
> in the 1.9.2 branch (which could have been a different bug of sorts).

It was originally reported using Firefox 3.6.8 for Thunderbird, but the version was not specified.
(In reply to comment #21)
> (In reply to comment #20)
> >
> > Ok, thanks, I just wanted to make sure we hadn't missed the fact that this was
> > in the 1.9.2 branch (which could have been a different bug of sorts).
> 
> It was originally reported using Firefox 3.6.8 for Thunderbird, but the version
> was not specified.

Ah, sorry - the version was specified - 3.1.2, so it was seen on 1.9.2.
blocking2.0: ? → -

Comment 23

6 years ago
See comment #7,
This should block Gecko 2.0.1(Firefox4.0.1),

Updated

6 years ago
Attachment #526680 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 24

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b895bc3e7a24 (patch)
http://hg.mozilla.org/mozilla-central/rev/1c819504b0bb (testcase)
(Assignee)

Comment 25

6 years ago
Comment on attachment 526665 [details] [diff] [review]
patch, v3 - use a bit flag in the font entry

This is a narrowly-targeted (and therefore minimal-risk) patch to work around a bug in the Italic faces of the widely-used Times New Roman font. It's important we take this for Fx5 along with the patch for bug 635639 (Arabic/Persian vowels), otherwise that fix will manifest as a regression - because of the buggy font - on pages that have "double quotes" in italic elements using default fonts on Windows.

Also re-nominating for gecko 2.0, as per comment 23. I'm not sure of the current schedule for Fx4.0.1, but if at all possible we should take this patch, as one of the Fx4 regressions we fixed there was bug 635639 - critical to Arabic-script users - but that means we'll regress this issue, which was "fixed" (hidden) in Fx4 release.
Attachment #526665 - Flags: approval2.0?
Attachment #526665 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 26

6 years ago
Updated platform and version, as this actually affects multiple versions and platforms (depending on fonts used) not just Windows/1.9.2 where it was originally reported. It is fixed on trunk, but the patch cannot readily be backported to pre-2.0 branches, see comment 18.
Status: NEW → RESOLVED
blocking2.0: - → ?
Last Resolved: 6 years ago
status-firefox5: --- → affected
OS: Windows Vista → All
Hardware: Other → All
Resolution: --- → FIXED
Version: 1.9.2 Branch → unspecified
(Assignee)

Comment 27

6 years ago
Comment on attachment 526680 [details] [diff] [review]
reftest to verify that double-quote in times italic has non-zero width

Also requesting approval for testcase, along with the actual patch.
Attachment #526680 - Flags: approval2.0?
Attachment #526680 - Flags: approval-mozilla-aurora?

Comment 28

6 years ago
We do not need to track this bug for a particular release and don't want it on mozilla-aurora. Now that it is fixed it will make it into the next FF release. tracking and approval-mozilla-aurora -
blocking2.0: ? → -
tracking-firefox5: ? → -
tracking-firefox6: ? → -

Updated

6 years ago
Attachment #526665 - Flags: approval2.0?
Attachment #526665 - Flags: approval2.0-
Attachment #526665 - Flags: approval-mozilla-aurora?
Attachment #526665 - Flags: approval-mozilla-aurora-

Updated

6 years ago
Attachment #526680 - Flags: approval2.0?
Attachment #526680 - Flags: approval2.0-
Attachment #526680 - Flags: approval-mozilla-aurora?
Attachment #526680 - Flags: approval-mozilla-aurora-
Comment on attachment 526665 [details] [diff] [review]
patch, v3 - use a bit flag in the font entry

I think we should reconsider this approval for Aurora; we missed a bit of detail during the triage meeting -- it fixes a regression that may be pretty bad and which we haven't actually shipped yet, though we're about to ship it in 4.0.1.
Attachment #526665 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
Attachment #526680 - Flags: approval-mozilla-aurora-
blocking2.0: - → ?
tracking-firefox5: - → ?
tracking-firefox6: - → ?
(Assignee)

Comment 30

6 years ago
+1 to that.

This issue manifests as a regression in text rendering for sites that have italic-styled text using Times New Roman, which is widespread on Windows as it's common as a default serif font. Although at root it's a font error, users will perceive it as a Firefox problem.

It's unfortunate that we're going to ship this regression in 4.0.1, but I realize it may be too late to fix that - I don't think it justifies holding the release, though if we need to respin the builds then I think it should be taken as a ridealong. But for Fx5 (currently Aurora) we are clearly in a position to be able to fix this without impacting schedules or taking unreasonable risk, and we should do so.

Comment 31

6 years ago
(In reply to comment #30)
> I don't think it justifies holding the release

A lot of users (including me) wouldn't agree with that.
Adding relnote keyword just in case.
Keywords: relnote
Summary: Times Roman Italic format error → Times New Roman Italic format error

Updated

6 years ago
Duplicate of this bug: 646397
To clarify the state of things, this bug was originally reported on the 1.9.2 branch, and we are OK not fixing it for them despite 100s of millions of users? As sucky as it is that we broke our 4.0 fix for it it doesn't sound like a stop-the-world re-spin kind of bug.

Comment 34

6 years ago
We won't respin Macaw/4.0.1 for this issue as it exists on 3.6.x. We understand that it will be a regression for existing 4.0 users. If we need to spin Macaw for another issue we will take this as a ridealong.

I think this should come in aurora though, approving for that release. That also makes it so the regression for 4.0 people only lasts until the end of june.
blocking2.0: ? → -

Updated

6 years ago
Attachment #526665 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 35

6 years ago
Pushed to Aurora:
http://hg.mozilla.org/mozilla-aurora/rev/66f453a26415
status-firefox5: affected → fixed
(In reply to comment #33)
> To clarify the state of things, this bug was originally reported on the 1.9.2
> branch, and we are OK not fixing it for them despite 100s of millions of users?
> As sucky as it is that we broke our 4.0 fix for it it doesn't sound like a
> stop-the-world re-spin kind of bug.

Based on my understanding of IRC discussion with jdaggett last night, it only affected 1.9.2 when we used the Uniscribe codepath, which, as I understand it, only happened when either:
 * text-rendering: optimizeLegibility was set
 * complex scripts were used (which perhaps doesn't intersect a lot with use of Times New Roman)
(Assignee)

Comment 37

6 years ago
(In reply to comment #36)

> Based on my understanding of IRC discussion with jdaggett last night, it only
> affected 1.9.2 when we used the Uniscribe codepath, which, as I understand it,
> only happened when either:
>  * text-rendering: optimizeLegibility was set
>  * complex scripts were used (which perhaps doesn't intersect a lot with use of
> Times New Roman)

Or when the font size was greater than some "quality" threshold, IIRC - so it was more likely to show up in titles than in body text, for example.

Yes, in 1.9.2 it didn't show up as frequently because a lot of TNR rendering would have gone through the "dumb" GDI path. (See also bug 573407 comment #2.) So the original problem in 1.9.2 and earlier is less serious than the regression in 2.0, as it shows up relatively rarely. Added to that is the fact that a workaround for 1.9.2 would be significantly more complex, and you have added clarification re comment #33.

Updated

6 years ago
tracking-firefox5: ? → ---

Updated

6 years ago
Duplicate of this bug: 653759

Updated

6 years ago
Duplicate of this bug: 654642

Updated

6 years ago
tracking-firefox6: ? → ---

Updated

6 years ago
Depends on: 729815
You need to log in before you can comment on or make changes to this bug.