Closed Bug 873902 Opened 6 years ago Closed 6 years ago

Display Arabic text with diacritics is bad

Categories

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

23 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 + verified

People

(Reporter: over68, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(3 files)

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
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.
Blocks: 863248
Keywords: regression
I can confirm this on my Win8 machine; these fonts render badly when diacritics are used.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Behdad, this is a result of f368ba4a9edec4e297616698097546e8e6c89e53. Reverting that change fixes the rendering here - but presumably makes Amiri worse again. :(
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).
Assignee: nobody → jfkthame
Attachment #751620 - Flags: feedback?(mozilla)
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.
We should also add a reftest based on the examples here.
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.
Attachment #751620 - Flags: feedback?(mozilla) → feedback+
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. :(
Ah, right.  *Those* ones.  I thought it broke old fonts.  Anyway, hopefully all stable now.  Fingers crossed.
The patch as landed upstream, including factoring out the width-zeroing code into helpers.
Attachment #751688 - Flags: review?(jdaggett)
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.)
Attachment #751690 - Flags: review?(jdaggett)
(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.
Attachment #751690 - Flags: review?(jdaggett) → review+
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?
(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.
Attachment #751688 - Flags: review?(jdaggett) → review+
Component: General → Layout: Text
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
Attachment #751688 - Flags: approval-mozilla-aurora?
Attachment #751688 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.