589682 - Times New Roman Italic format error

Status: RESOLVED FIXED

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

Description

[Gary]

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

Comment 1

[Ludovic Hirlimann [:Usul]]

Can we get a screenshot ?

Comment 2

[Vykintas Vyšniauskas]

Attachment: testcase

Comment 3

[Vykintas Vyšniauskas]

Attachment: What I see with the testcase

Comment 4

[Vykintas Vyšniauskas]

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).

Comment 5

[Vykintas Vyšniauskas]

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

[Alice0775 White]

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

Comment 7

[Alice0775 White]

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

Comment 8

[Jonathan Kew [:jfkthame]]

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

[Alice0775 White]

(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

Comment 10

[Jonathan Kew [:jfkthame]]

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.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 12

[Jonathan Kew [:jfkthame]]

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.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: 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).

Comment 14

[John Daggett (:jtd)]

Attachment: 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

[John Daggett (:jtd)]

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.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 17

[Mark Banner (:standard8)]

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.

Comment 18

[Jonathan Kew [:jfkthame]]

(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.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 20

[Mark Banner (:standard8)]

(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).

Comment 21

[Vykintas Vyšniauskas]

(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.

Comment 22

[Vykintas Vyšniauskas]

(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.

Comment 23

[Alice0775 White]

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

Comment 24

[Jonathan Kew [:jfkthame]]

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

Comment 25

[Jonathan Kew [:jfkthame]]

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.

Comment 26

[Jonathan Kew [:jfkthame]]

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.

Comment 27

[Jonathan Kew [:jfkthame]]

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.

Comment 28

[LegNeato]

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 -

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 30

[Jonathan Kew [:jfkthame]]

+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

[j.j.]

(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.

Comment 33

[Daniel Veditz [:dveditz]]

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

[LegNeato]

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.

Comment 35

[Jonathan Kew [:jfkthame]]

Pushed to Aurora:
http://hg.mozilla.org/mozilla-aurora/rev/66f453a26415

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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)

Comment 37

[Jonathan Kew [:jfkthame]]

(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.