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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Uri Bernstein]

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.

Comment 1

[Uri Bernstein]

Attachment: testcase

The first three rows in the table use MS fonts, and the parentheses are reversed.

Comment 2

[John Daggett (:jtd)]

Attachment: simpler version of testcase with just two rows of Hebrew text

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.

Comment 3

[John Daggett (:jtd)]

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)

Comment 4

[Simon Montagu :smontagu]

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.

Comment 5

[John Daggett (:jtd)]

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.

Comment 6

[John Daggett (:jtd)]

WebKit bug fixed back in 2005:

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

Apple Bug Number (Radar):

rdar://problem/4215911

Comment 7

[John Daggett (:jtd)]

Attachment: initial patch v. 0.01

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.

Comment 8

[John Daggett (:jtd)]

Attachment: patch, v. 0.02

Comment 9

[John Daggett (:jtd)]

Attachment: patch, v. 0.03, tabs and silly comments removed

Comment 10

[John Daggett (:jtd)]

Attachment: patch, v. 0.1, after stuart's quick check

Comment 11

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 12

[John Daggett (:jtd)]

Attachment: testpage with all system and library fonts

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.

Comment 13

[John Daggett (:jtd)]

Attachment: patch, v. 0.9, based on 385539 changes

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.

Comment 14

[John Daggett (:jtd)]

Attachment: patch, v. 1.0, spacing omitted

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.

Comment 15

[John Daggett (:jtd)]

Note: bug does *not* occur in latest WebKit nightly

WebKit r23841 20070628

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 17

[John Daggett (:jtd)]

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

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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