873902 - Display Arabic text with diacritics is bad

Status: VERIFIED FIXED

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

Description

[blinky]

Does not display Arabic text with diacritics correctly in https://dl.dropboxusercontent.com/u/95157096/85f61cf7/e5f68Ja10.html

Screenshot: http://img705.imageshack.us/img705/7801/ca3b10d9.png

This happens since the version 23.0a1 http://hg.mozilla.org/mozilla-central/rev/02aa81c59df6

Comment 1

[Jonathan Kew [:jfkthame]]

I guess this must be a regression from bug 863248 (update harfbuzz to 0.9.16). Behdad, could you take a look? It appears the update isn't working well at least with older (possibly no-GPOS?) Arabic fonts.

Comment 2

[Jonathan Kew [:jfkthame]]

I can confirm this on my Win8 machine; these fonts render badly when diacritics are used.

Comment 3

[Jonathan Kew [:jfkthame]]

Behdad, this is a result of f368ba4a9edec4e297616698097546e8e6c89e53. Reverting that change fixes the rendering here - but presumably makes Amiri worse again. :(

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, zero mark widths after positioning for Arabic

Note that the problem isn't really the use of GDEF vs Unicode for zeroing marks; it's the difference between zeroing them "early" (before GPOS) or "late" (after).

Possible patch attached (also posted to the harfbuzz list).

Comment 5

[Jonathan Kew [:jfkthame]]

This is a serious enough regression for Arabic script that we'll need to uplift a fix to FF23 once it's landed on trunk.

Comment 6

[Jonathan Kew [:jfkthame]]

We should also add a reftest based on the examples here.

Comment 7

[Behdad Esfahbod]

Comment on attachment 751620 [details] [diff] [review]
patch, zero mark widths after positioning for Arabic

Review of attachment 751620 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  I can't quite explain the results we see before the patch, but the patch looks right and I committed it upstream.

Comment 8

[Jonathan Kew [:jfkthame]]

I think the explanation is that in these fonts, the mark glyphs are designed with a non-zero inherent advance, and then GPOS is used to cancel out that width by negative kerning or adjustment; but this means that if we've already zeroed it *before* applying GPOS, we end up with drastically *negative* advances wherever the marks occur, resulting in unreadable ugliness. :(

Comment 9

[Behdad Esfahbod]

Ah, right.  *Those* ones.  I thought it broke old fonts.  Anyway, hopefully all stable now.  Fingers crossed.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: zero mark widths after GPOS positioning for Arabic fonts

The patch as landed upstream, including factoring out the width-zeroing code into helpers.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: reftest for Arabic marks with Windows fonts

Testcase to detect this kind of breakage: verifies that Arabic text with diacritics is the same width as without, using several of the Windows fonts that fail in this manner. (Courier New has the same issue on OS X, too.)

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Behdad Esfahbod from comment #9)
> Ah, right.  *Those* ones.  I thought it broke old fonts.

Yeah, sorry, I said "older fonts" above but it actually fails with some of the current ones, even shipping on Win8. (I think it's a crazy way to engineer the fonts, but oh well....)

Just FTR: the proposed reftest (attachment 751690 [details] [diff] [review]) uses the text "الرَّحْمَنِ الرَّحِيمِ" from the example, but deliberately omits the "بِسْمِ اللَّهِ" portion. This is because in many fonts, the presence of the diacritics will disrupt formation of the "الله" ligature, and so affect the total width of the text even when shaping works correctly. Using only the second part of the phrase as the testcase avoids that issue.

Comment 13

[Jonathan Kew [:jfkthame]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=985ae08ee397

Comment 14

[John Daggett (:jtd)]

Comment on attachment 751688 [details] [diff] [review]
zero mark widths after GPOS positioning for Arabic fonts

> +//  HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_EARLY,
> 
> -    case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_GDEF:
> +    //case HB_OT_SHAPE_ZERO_WIDTH_MARKS_BY_UNICODE_EARLY:

Did you mean to leave these commented out values here?

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #14)

> Did you mean to leave these commented out values here?

Yes - it's a placeholder for a potential setting, but no shaper currently uses it, so there's no need to actually have the implementation present in the code for now.

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689a40de8c5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/54553d06c80d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/689a40de8c5a
https://hg.mozilla.org/mozilla-central/rev/54553d06c80d

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 751688 [details] [diff] [review]
zero mark widths after GPOS positioning for Arabic fonts

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 863248

User impact if declined: garbled display of Arabic-script text when diacritics are present, in some common fonts (esp. on Windows)

Testing completed (on m-c, etc.): on Nightly for several days; also added testcase to unit tests

Risk to taking this patch (and alternatives if risky): low-risk fix undoing an inadvertent behavior change in the harfbuzz update; already landed in the upstream repository

String or IDL/UUID changes made by this patch: none

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7583a702dab

Comment 20

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on Firefox 23 beta 8:
User agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID 20130722172257.