378351 - Diacritics shift to the left in RTL text

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Simon Montagu :smontagu]

Attachment: Minimal testcase

This is a regression from the new textrun: diacritics in RTL text are positioned too far to the left. It doesn't happen for all fonts or all combinations of base letter and diacritic, but where it does happen it makes the text almost unreadable.

Explanation for non-Hebrew readers: in the attached testcase the small t-shaped diacritical mark (U+05B8 HEBREW POINT QAMATS) should be positioned centrally below the large three-pronged base character (U+05E9 HEBREW LETTER SHIN).

Comment 1

[Simon Montagu :smontagu]

Attachment: patch

This fixes the bug for me on Windows, not yet tested on other platforms.

Comment 2

[Uri Bernstein]

On OS X:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a4pre) Gecko/20070421 Minefield/3.0a4pre ID:2007042104 [cairo]

The qamats is positioned way too far to the *right* (almost entirely off-screen).

The patch doesn't seem to help here: details->mXOffset is 0 whenever the patched line is reached.

Comment 3

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

I'd prefer to keep mXOffset as strictly left-to-right, because there's a lot of code that assumes that, and fix this on Windows by reversing the x-offset for RTL text when we store it in the textrun.

We should also add a comment here:
http://lxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxFont.h#961
mentioning that mAdvance is in the text direction but mXOffset is not.

Comment 4

[Simon Montagu :smontagu]

(In reply to comment #2)
> On OS X:
> The patch doesn't seem to help here: details->mXOffset is 0 whenever the
> patched line is reached.

Yeah, mXOffset is always set to 0 in gfxAtsuiFonts.cpp. Do combining characters work correctly in LTR text?

Comment 5

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

I couldn't figure out how to get x offset information from ATSUI's layout. I think perhaps it just draws a glyph with a zero advance and then draws another glyph with the real advance.

Comment 6

[Simon Montagu :smontagu]

(In reply to comment #3)
> I'd prefer to keep mXOffset as strictly left-to-right, because there's a lot of
> code that assumes that, and fix this on Windows by reversing the x-offset for
> RTL text when we store it in the textrun.

This is tricky if we care about text with a left-to-right override: the sign of the x-offset returned by ScriptPlace() depends on whether the *script* is right-to-left rather than whether the run is. The same goes for the original patch, of course.

Comment 7

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

Then it's tricky whenever we need to override the script direction, no matter which approach we take, right?

We do tell Uniscribe what the true direction is. That's not enough?

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch v.2

Comment 6 is wrong: the problem only seems to occur in legacy fonts without OT tables, and they don't have very accurate diacritic placement anyway. With this patch we have parity with IE7 and I think that's good enough.

Comment 9

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

Comment on attachment 262616 [details] [diff] [review]
Patch v.2

Just do
details->mXOffset = float(mOffsets[k + i].du)*appUnitsPerDevUnit*aRun->GetDirection()

Comment 10

[Simon Montagu :smontagu]

Checked in.

Comment 11

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).