Closed Bug 361695 Opened 13 years ago Closed 13 years ago

[Mac][cairo] Bidi mirroring (e.g. for parentheses) broken with Microsoft fonts (e.g. Arial)

Categories

(Core :: Graphics, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: uriber, Assigned: jtd)

References

()

Details

(Keywords: regression, testcase)

Attachments

(4 files, 5 obsolete files)

Characters that should be mirrored in RTL text (e.g.: (), [], <>) are no longer mirrored when using Microsoft fonts (Times New Roman, Arial, Verdana), since cairo was enabled.

Testcase coming up.
Attached file testcase
The first three rows in the table use MS fonts, and the parentheses are reversed.
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9alpha6
Assignee: nobody → jdaggett
Simple testcase with two rows of Hebrew text, first row uses 'Times New Roman', second row uses 'Times'.  On Mac OS X 10.4, 'Times New Roman' contains glyphs for Hebrew while Times does not.

The code within gfxAtsuiFontGroup::InitTextRun that handles font substitution creates a single GlyphRun for the first case but breaks apart the string into three GlyphRun's in the second case, because it needs to substitute another font with Hebrew glyphs in place of Times.  As a side effect of this, the first case renders incorrectly but the second one correctly.
The handling of glyph mirroring on the Mac is handled by the code in gfxAtsuiFontGroup::InitTextRun.  Within the WebKit code, the file WebKit/WebCore/platform/mac/FontMac.mm contains code that does very similar operations.  The function initializeATSUStyle initializes a variable m_ATSUMirrors based on the existance of a 'prop' table in the font.  If no 'prop' table exists for the font, the code manually performs the Unicode character mirroring when handling a RTL text run.  We don't have any code like this, it appears that ATSUI doesn't do glyph mirroring for RTL layouts that use fonts that don't contain a 'prop' table.

This seems lame, ATSUI should be doing this. *sigh*  In Mac OS 10.4.9, most of the fonts in /System/Library/Fonts have 'prop' tables but only half or so of the fonts in /Library/Fonts have 'prop' tables.

To grab the WebKit source:

http://webkit.org/building/checkout.html

For viewing the contents of font tables, you need to install the Apple Font Tools.

Apple Font Tools 3.1.0
http://developer.apple.com/textfonts/fonttools/AppleFontToolSuite3.1.0.dmg.zip

To show the tables contained in a font:
ftxdumperfuser -l /System/Library/Fonts/Geneva.dfont

To dump the contents of the 'prop' table in XML format:
ftxdumperfuser -o dump.xml -t prop /System/Library/Fonts/Geneva.dfont

Fonts with Hebrew glyphs on 10.4.9:

Arial (no prop)
Corsiva (prop)
Lucida Grande (prop)
Times New Roman (no prop)
Arial Hebrew (prop)
New Peninim MT (prop)
Raanana (prop)

Fonts without Hebrew glyphs:

Times (prop)
Geneva (prop)
Helvetica (prop)
Courier (prop)
Verdana (no prop)

Status: NEW → ASSIGNED
I don't think this needs to be such a big deal. We have a method in the tree, nsBidi::SymmSwap, to do mirroring for platforms that don't do it themselves. Currently it's only called for platforms that don't reverse the text on display, i.e. never (under Cairo), but we could just hook it into the Mac glyph extraction.
The problem is that this is not universal but per font, so the decision to do mirroring or not has to be made *after* font substitution has been done.  Without this, ATSUI will do the swapping for fonts that contain 'prop' tables and will end up displaying incorrectly.
WebKit bug fixed back in 2005:

http://bugs.webkit.org/show_bug.cgi?id=3435

Apple Bug Number (Radar):

rdar://problem/4215911

Attached patch initial patch v. 0.01 (obsolete) — Splinter Review
First attempt at a fix.  Patch includes a dupe of some Unicode mirroring code that needs to be moved from layout into xpcom/base.  Still need to put together some good reftest's for regression testing.
Attached patch patch, v. 0.02 (obsolete) — Splinter Review
Attachment #269450 - Attachment is obsolete: true
Attachment #269452 - Attachment is obsolete: true
Depends on: 385539
Attachment #269454 - Attachment is obsolete: true
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Note: for some reason ATSUI ignores the mirroring information contained in the prop tables for Osaka and Osaka Mono.  Safari has the same problem.  These are Japanese fonts, so this probably isn't such a big deal.
Note: the logic of the patch works for all fonts with the exception of Osaka and Osaka Mono.  Since Safari has the same problem, seems like this is an ATSUI/font problem.  Both of these fonts are Japanese fonts so the chance of users specifying this font in RTL text is pretty slim.
Attachment #269458 - Attachment is obsolete: true
Attachment #270374 - Flags: review?(vladimir)
The Osaka/Osaka-Mono problem appears related to the use of the TT 'mort' table.  This table contains glyph morphing information.  Originally developed as part of QDGX, this was replaced by the TT 'morx' table in OS X.  The OT equivalent are the GSUB and GPOS tables.

Osaka parentheses mapping:

cmap table info

'(' 0x28 ==> glyph ref 38
')' 0x29 ==> glyph ref 39

mort table info
non-contextual subtable

glyph ref 38 ==> 8031
glyph ref 39 ==> 8032

These are the glyph ref's that ATSUI hands back to us in PostLayoutCallback.  The 'prop' table contains mirroring information for these glyphs but for some reason ATSUI does not apply the mirroring to the substituted glyphs.

We could special case Osaka / Osaka-Mono in the RTL case but since the problem is related to older-style TT 'mort' tables, I don't think we'll see this problem going forward.  My guess is a lot of this will be reworked anyways as part of the rumored CoreText update that will replace ATSUI in the future.
Attachment #270374 - Attachment is obsolete: true
Attachment #270815 - Flags: review?(vladimir)
Attachment #270374 - Flags: review?(vladimir)
Note: bug does *not* occur in latest WebKit nightly

WebKit r23841 20070628
Comment on attachment 270815 [details] [diff] [review]
patch, v. 1.0, spacing omitted

r=me, looks fine.. one questions though, is it ok to continue modifying the string after calling ATSUTextMoved?  You may want to just move that call out of the loop and just do it at the end of the function.

Let me know what you think about that change and I can check this in for you later.
Attachment #270815 - Flags: review?(vladimir) → review+
I'm assuming it is, since were really not asking ATSUI to do anything other than font matching with the string until ATSUGetGlyphBounds is called.  Moving it out of the loop doesn't prevent changes to the string occurring after ATSUTextMoved is called, since MirrorSubstring may be called multiple times if fonts are substituted midstring, e.g. with Arial in the testcase.

WebKit is doing the same thing, copying the entire string on the first character that needs to be swizzled, then continuing with the font substitution for the rest of the string:

http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/platform/mac/FontMac.mm#L419
Checked in:

Checking in thebes/public/gfxAtsuiFonts.h;
/cvsroot/mozilla/gfx/thebes/public/gfxAtsuiFonts.h,v  <--  gfxAtsuiFonts.h
new revision: 1.32; previous revision: 1.31
done
Checking in thebes/src/gfxAtsuiFonts.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp,v  <--  gfxAtsuiFonts.cpp
new revision: 1.53; previous revision: 1.52
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.