532346 - Apple LiGothic rendering failure when encounter specific characters

Status: RESOLVED FIXED

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

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Text page: the same parapragh with different fonts on Mac.

Reproducible: Always

Steps:
* See attachment for test paragraphs with font-family designate to specific font Mac

Result:
* Apple LiGothic test failed with characters following the trigger phrase "眼睛" (i.e. "eye(s)") overlaps previous characters. There are other known trigger phrases but we haven't find out all of them.

This bug is serious as LiGothic is currently the default zh-TW font in pref. 

Also Firefox failed to show paragraphs in Heiti TC (The new zh-TW font shipped with 10.6) whatever font-family was written, but this belongs to another bug.

Tim

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Rendering failure: Fx3.6b4, OS X 10.6

Comment 2

[Irvin (MozTW)]

Attachment: Fx 3.5.5 works without problem in OS X 10.4

Comment 3

[Irvin (MozTW)]

Comment on attachment 415657 [details]
Fx 3.5.5 works without problem in OS X 10.4

Fx 3.5.5 works without problem in OS X 10.4

Comment 4

[Irvin (MozTW)]

Attachment: Fx 3.6b4 also rendering correct in OS X 10.4

Comment 5

[Jonathan Kew [:jfkthame]]

Likely to be an issue with the Core Text back-end; I'll try to reproduce and track this down.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to comment #5)
> Likely to be an issue with the Core Text back-end; I'll try to reproduce and
> track this down.

Duh. Sorry, I'm confused. We're not compiling the CoreText backend in our official 3.6 builds, even though it's in the tree, because they are built with the 10.4 SDK and still run on OS X 10.4.

Running a trunk (3.7pre) nightly, which *does* have the CoreText backend, the testcase renders correctly. Using about:config to switch back to ATSUI in that build causes the problem to reappear.

So this appears to be a problem with the ATSUI back-end on 10.6 only, which suggests it may be a regression in ATSUI itself rather than our bug. I'll try to investigate further and confirm this.

Comment 7

[philippe (part-time)]

Fwiw, Gecko 1.9.0.17pre (Camino nightly) and Gecko 1.9.1.x also render the test incorrectly on 10.6.
Renders correctly on 10.5.8

Console notes this, multiple times:
12/3/09 11:10:27 AM
[0x0-0x321321].org.mozilla.camino[30978]
Faulty glyph (id:3774) outline detected - replacing with a space/null glyph - /Library/Fonts/Apple LiGothic Medium.ttf

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does this affect Safari? If not, is that because Safari is using Core Text on 10.6?

Should we work around this bug by switching our font prefs to avoid this font?

Can we work around it in the ATSUI backend somehow?

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to comment #0)
> Also Firefox failed to show paragraphs in Heiti TC (The new zh-TW font shipped
> with 10.6) whatever font-family was written, but this belongs to another bug.
FYI, this bug is now bug 532349.

(In reply to comment #8)
> Should we work around this bug by switching our font prefs to avoid this font?
We could, and I nominate "LiHei Pro" as the choice, but this should be consider a temporary fix until we finally move to CoreText for all products.

Just in case, please DO NOT switch font prefs to Heiti TC, that font was poorly made and is unpopular among Taiwanese Mac users and designers. There are even a tool (http://zonble.github.com/tcfail/) dedicate to change the system font back to LiHei Pro in 10.6; not to mention due to bug mentioned above it's not even currently possible.

Again, thank you for your attention to the LiGothic rendering problem.

Tim

Comment 10

[Irvin (MozTW)]

If we want to change font prefs and avoid the issue, the reasonable choice must be "Heiti TC", new standard font come with OS X 10.6 as default trad. chinese system font.

But the font is unable to use (weather though CSS or config) as another bug we discovered at same time (bug 532349).

Also there is some argument about "Heiti TC" as the default font across Trad. Chinese Mac user, mainly come from some instandard glyphs of the font. See http://zonble.github.com/tcfail/en.html for more info.

Thus. I think the Trad. Chinese mac Firefox users would glad to have the font remaining the same as before (Apple LiGothic) than switch to something new but horrible one (ie. Heiti TC), if we can discover some method and solving the rendering problem.

Comment 11

[Jonathan Kew [:jfkthame]]

The rendering failure is caused by a flaw in the Apple LiGothic font, which apparently causes ATSUI to reject the glyph for U+775B (睛) when getting glyph metrics, although the glyph image still gets rendered when painting the text. The result is incorrectly spaced, overprinted text.

By patching that glyph in the font, I am able to fix the problem, and the text renders correctly with FF 3.5.5 on 10.6.2. However, we can't deploy that solution.

I have filed rdar://7440270 to report the font/ATSUI issue to Apple, and will see if I can come up with a way to work around the problem in our ATSUI back-end.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to comment #8)
> Does this affect Safari? If not, is that because Safari is using Core Text on
> 10.6?

Safari is unaffected; I haven't examined the WebKit code to see what they're using, but I'd expect it to be based on either Core Text or Cocoa-based text support. Most system apps such as TextEdit will be using NSTextView etc, which does not show the problem.

OTOH, I can reproduce the problem in XeTeX, which uses ATSUI for low-level text layout in a similar way to our ATSUI code in Thebes.

> Should we work around this bug by switching our font prefs to avoid this font?

That would help, but there'd still be the possibility of pages that explicitly list it in their CSS.

> Can we work around it in the ATSUI backend somehow?

Investigating....

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: patch v1: avoid char U+0x775B in Apple LiGothic font

This patch is a very lightweight workaround: it simply masks off the problem codepoint (U+775B) in the LiGothic cmap on 10.6, with the result that we fall back to a different font for that character. This gives readable text, but the glyph will not blend perfectly with the rest of the text.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: screenshot showing testcase rendered with patch v1

This shows result of the testcase using the v1 patch, where U+775B is rendered using a fallback font to avoid the LiGothic/ATSUI rendering bug.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: patch v2: hack to get proper metrics for U+0x775B and following chars

This more invasive patch works around the problem by substituting an alternate character (with the same metrics) in place of U+775B during the ATSUI layout process, then replacing the resulting glyph in the textRun after we have the final glyph positions.

Because of the possibility that Apple may update/fix the font and the glyph IDs could change at that time, this includes code to check the LiGothic cmap, and will only apply the hack if the mapping for U+775B matches the expected glyph ID.

IMO, the v1 patch would be sufficient to let us unblock on this bug, as it gives readable text, but this version is preferable as it avoids the typographic mismatch of using a fallback font for one character in the text.

(Patch is developed on 1.9.1, as that is where I can readily reproduce the issue; we will also need it on 1.9.2, and on trunk until we remove ATSUI support.)

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: screenshot showing testcase rendered with patch v2

This shows the testcase rendered with FF 3.5 + patch v2; compare with the v1 screenshot to see the difference in rendering of U+775B 睛 (the second character within the first set of 「quotes」 on the LiGothic line).

Comment 17

[Irvin (MozTW)]

Attachment: screenshot of other randering error words

One concern on the patch is that we're not sure there are only this character (U+775B) has the rendering problem. In fact, we'd discover many characters, but  can't locate the others exactly, 'cause the rendering error happened in random. 

For example, the attachment shows "栽培" & "脖子" had rendering in error, but these words work normally in most of the time and most websites, I can't reproduce the error on these words.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (hackForLiGothicAtsuiBug.Length() > 0) {
+        for (PRUint32 i = 0; i < hackForLiGothicAtsuiBug.Length(); ++i) {

I think the outer 'if' statement is not helpful.

+    if ((gfxPlatformMac::GetPlatform()->OSXVersion() & MAC_OS_X_MAJOR_VERSION_MASK) == MAC_OS_X_VERSION_10_6_HEX) {
+        // even ruder hack - the LiGothic font on 10.6 has a bad glyph for U+775B that causes ATSUI failure,
+        // so we set a flag to tell our layout code to hack around that character
+        if (mName.EqualsLiteral("LiGothicMed")) {
+            // check whether the problem char maps to the expected glyph;
+            // if not, we'll assume this isn't the problem version of the font
+            if (gfxFontUtils::MapCharToGlyph(cmap, cmapSize, kLiGothicAtsuiBadCharUnicode) == kLiGothicAtsuiBadCharGlyph) {
+                mUseLiGothicAtsuiHack = PR_TRUE;
+            }

Some major 80-char line length issues

Do we actually need both of MapCharToGlyphFormat12 and MapCharToGlyphFormat4? Or can we get away with just the version that LiGothic uses?

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to comment #17)
> Created an attachment (id=416713) [details]
> screenshot of other randering error words
> 
> One concern on the patch is that we're not sure there are only this character
> (U+775B) has the rendering problem. In fact, we'd discover many characters, but
>  can't locate the others exactly, 'cause the rendering error happened in
> random. 
> 
> For example, the attachment shows "栽培" & "脖子" had rendering in error, but these
> words work normally in most of the time and most websites, I can't reproduce
> the error on these words.

My justification for patching specifically for U+775B is that examining the font data shows that this glyph (and no other glyph in the font) has a specific anomaly - a "null" contour with no points at the end of the outline data - which I guess is triggering the ATSUI layout failure. It's not clear to me whether this is strictly legal in TrueType, but it would certainly be an unexpected edge case.

Note that the presence of U+775B appears to disrupt the rendering of not only that character but also following glyphs in the text run.

If you're seeing separate, non-reproducible rendering failures where U+775B is not involved, there may be other issues as well. One possibility may be font cache corruption; I suggest using FontNuke or a similar tool to clear all the system font caches, and see if this resolves those problems. Also, please check for any error messages in Console.app (see comment #7).

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to comment #18)
> +    if (hackForLiGothicAtsuiBug.Length() > 0) {
> +        for (PRUint32 i = 0; i < hackForLiGothicAtsuiBug.Length(); ++i) {
> 
> I think the outer 'if' statement is not helpful.

True; I think that was left from an earlier approach.

> +    if ((gfxPlatformMac::GetPlatform()->OSXVersion() &
> MAC_OS_X_MAJOR_VERSION_MASK) == MAC_OS_X_VERSION_10_6_HEX) {
> +        // even ruder hack - the LiGothic font on 10.6 has a bad glyph for
> U+775B that causes ATSUI failure,
> +        // so we set a flag to tell our layout code to hack around that
> character
> +        if (mName.EqualsLiteral("LiGothicMed")) {
> +            // check whether the problem char maps to the expected glyph;
> +            // if not, we'll assume this isn't the problem version of the font
> +            if (gfxFontUtils::MapCharToGlyph(cmap, cmapSize,
> kLiGothicAtsuiBadCharUnicode) == kLiGothicAtsuiBadCharGlyph) {
> +                mUseLiGothicAtsuiHack = PR_TRUE;
> +            }
> 
> Some major 80-char line length issues

Just fitting in with the surrounding code! :)  Ok, I'll shorten them.

> Do we actually need both of MapCharToGlyphFormat12 and MapCharToGlyphFormat4?
> Or can we get away with just the version that LiGothic uses?

As the only current use of MapCharToGlyph is for this hack, we could get away with just providing Format 4 support. I included both for completeness, as we recognize both formats in ReadCMAP, but it's not strictly necessary here.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 416711 [details] [diff] [review]
patch v2: hack to get proper metrics for U+0x775B and following chars

OK, let's take out the format-12 support for now and just return an error or something if format-12 is encountered, so that we don't apply the hack.

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: patch v2.1: addressed review comments, ported to trunk for initial landing

Comment 23

[Jonathan Kew [:jfkthame]]

Pushed http://hg.mozilla.org/mozilla-central/rev/91d3b21e47bc

Comment 24

[Irvin (MozTW)]

This bug also applied to Prism 10.b2,
Thanks everyone for working on this :)

Comment 25

[Jonathan Kew [:jfkthame]]

Pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/60cf6f6fc3c2

Comment 26

[Jeff Wang]

Attachment: add LP64 check to MacPlatformFontList

The current code breaks 64bit builds on OS X.

The problem is kLiGothicBadCharUnicode and kLiGothicBadCharGlyph are defined within the __LP64__ check whereas gfxMacPlatformFontList.mm is missing a check and thus fails to compile at lines 277 and 278.

if (gfxFontUtils::MapCharToGlyph(cmap.Elements(), cmap.Length(),
                                             kLiGothicBadCharUnicode) ==
                kLiGothicBadCharGlyph) {

I just added a #ifndef to check if it's 64bit.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 417422 [details] [diff] [review]
add LP64 check to MacPlatformFontList

Yes, the check is fine, as we don't need any of this code on 64-bit.

Please remove the <tab> characters on the blank lines, though. We don't use tabs in our code; and we also try to avoid trailing whitespace.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: updated 64-bit build fix

Actually, we should also add #ifndef tests to eliminate the ATSUI hack flag completely from 64-bit builds. Updated the patch to include this.

Passing r? to joshmoz for the 64-bit Mac-ness here, as I don't actually have a 64-bit build set up.

[e-mail], if you could provide your real name, we'll include that in the patch when it lands.

Comment 29

[Jeff Wang]

Updated my profile.  Thanks.

Comment 30

[Josh Aas]

pushed 64-bit bustage fix to mozilla-central crediting Jeff with the fix

http://hg.mozilla.org/mozilla-central/rev/d1ea8d27b7e2

Thanks!

Comment 31

[Jonathan Kew [:jfkthame]]

Attachment: updated patch for gecko 1.9.1

Requesting approval for 1.9.1 also; I think we should take this for the sake of Chinese FF3.5 users who update to Snow Leopard, and will otherwise encounter this text rendering glitch.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 420062 [details] [diff] [review]
updated patch for gecko 1.9.1

gonna wait for 3.6rc to ship

Comment 33

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

Marking the 14 bugs that are both:
 * nominated for blocking1.9.3:?
 * fixed on the 1.9.2 branch (according to status1.9.2)
as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually.  They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 420062 [details] [diff] [review]
updated patch for gecko 1.9.1

Approved for 1.9.1.9, a=dveditz for release-drivers

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Dears,

It seems fixes on this bug does not cover text rendered in XUL UIs and textboxes. Our Mac beta testers had filed separate bug for that. (bug 543782)

And ideas?


Tim

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's strange, it should work there...

Comment 37

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6aa93ddde3db

Comment 38

[Andrea Govoni]

(In reply to comment #11)
> I have filed rdar://7440270 to report the font/ATSUI issue to Apple

Do you know about the Open Radar project? :)
Please fill the bug details if you like:
http://openradar.appspot.com/7440270