Last Comment Bug 706888 - Arabic joining does not work properly on the native UI
: Arabic joining does not work properly on the native UI
Status: RESOLVED FIXED
: intl
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: P3 normal (vote)
: mozilla14
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
http://bbcpersian.com/
Depends on: 758257
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-01 10:41 PST by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2012-05-24 15:13 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
12+


Attachments
screenshot from xul and native (313.11 KB, image/png)
2011-12-05 11:00 PST, Kevin Brosnan [:kbrosnan]
no flags Details
screenshot (107.16 KB, image/png)
2012-01-03 12:31 PST, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details
My /system/fonts directory contents (2.51 MB, application/octet-stream)
2012-01-03 15:36 PST, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details
patch, ignore GSUB in broken version of Droid Sans Arabic (1.58 KB, patch)
2012-04-24 01:38 PDT, Jonathan Kew (:jfkthame)
jd.bugzilla: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2011-12-01 10:41:55 PST
It works for some of the headlines on this page, so I suspect that it might be related to fonts somehow.  I don't remember having this problem on the XUL version.
Comment 1 Aaron Train [:aaronmt] 2011-12-05 09:32:22 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #0)
> It works for some of the headlines on this page, so I suspect that it might
> be related to fonts somehow.  I don't remember having this problem on the
> XUL version.

Does it happen on XUL?
Comment 2 Kevin Brosnan [:kbrosnan] 2011-12-05 11:00:29 PST
Created attachment 579121 [details]
screenshot from xul and native

I might be missing something but short of the address bar the the text looks the same.
Comment 3 Jonathan Kew (:jfkthame) 2011-12-05 13:33:40 PST
The page content looks fine; however, the address bar is thoroughly broken. This is a pretty serious functional regression that we're going to suffer as a result of the switch to native UI, but fixing it will be tricky on devices that don't natively support Arabic (or other complex-shaping scripts).
Comment 4 Doug Turner (:dougt) 2011-12-05 18:17:03 PST
Do we know if this is a problem on Android device that have their OS defaulted to Arabic?
Comment 5 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-12-06 00:32:52 PST
ehsan, which android OS and device are you on?  arabic isn't officially supported in versions less than 3.  http://blog.afriza.co.cc/archives/23
Comment 6 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-12-06 00:35:46 PST
correction: wasn't officially support until honeycomb.  It now is.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-12 14:17:26 PST
This is an Android issue
Comment 8 Jonathan Kew (:jfkthame) 2011-12-12 14:49:49 PST
It's still a Firefox regression between the XUL version and the Android-native version, though, as we used to be able to handle this.
Comment 9 Axel Hecht [:Pike] 2011-12-24 04:38:58 PST
http://colincooper.net/?p=238 mentions that arabic is in 2.3, confirmed by http://developer.android.com/sdk/android-3.2.html#locs.

That doesn't mean that it's not without glitches. Hindi for example is having a split story in terms of rendering, some devices seem to be doing it OK, though stock seems to be broken in rendering.
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-03 12:27:36 PST
I'm on Gingerbread based CyanogenMod, which has correct system-wide support for Arabic shaping.  But I was not talking about the parts of the UI drawn by Android, I was talking about the content area part drawn by Gecko.

In today's nightly, here's the behavior I get:

The shaping is broken on bbcpersian.com as it is loaded.  It gets fixed when it's finished loading.

The shaping is completely broken on Google search.

I will attach a screenshot.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-03 12:31:01 PST
Created attachment 585510 [details]
screenshot
Comment 12 Jonathan Kew (:jfkthame) 2012-01-03 15:09:00 PST
I think the issue here is that your device has a font that includes Arabic glyphs but lacks OpenType layout tables, so it doesn't really support Arabic-script rendering. The bbcpersian site uses a web font, so once that loads, it renders OK, but sites that rely on the device's local font support will be broken. See also bug 676068.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-01-03 15:36:45 PST
Created attachment 585584 [details]
My /system/fonts directory contents
Comment 14 Jonathan Kew (:jfkthame) 2012-01-04 01:37:50 PST
OK, so the problem is that your device has a bad DroidSansArabic font. Specifically, the font includes GDEF/GSUB/GPOS tables, which should support OpenType Arabic shaping, but those tables are completely wrong - the substitution rules, for example, refer to non-existent glyph IDs.

This can be confirmed by opening the .ttf file with FontForge, for example - it spews a huge number of warnings about nonexistent glyphs as it loads the OpenType tables - or by running it through Microsoft's Font Validator.

My guess is that this DroidSansArabic font was created by taking a "complete" DroidSans (including Arabic glyphs and with presumably-correct OpenType tables), and stripping out the non-Arabic glyphs. This means that the remaining glyphs have completely different glyph IDs than they had in the original font - but the OpenType tables were not updated to match. So harfbuzz Arabic shaping necessarily fails.

The reason the font works in native Android apps is almost certainly because the Java code implements Arabic shaping based on the (deprecated) Unicode presentation forms, ignoring the OpenType layout tables. That's a poor solution, as it cannot take advantage of all the features of OpenType fonts (e.g. contextual variants of the glyphs, beyond the basic init/medi/fina/isol forms), nor can it support all Arabic-script languages (as the repertoire of presentation forms is limited and will not be extended to cover newer Arabic-script letters used in minority languages).

This is similar to the situation in bug 676068, but it's actually somewhat worse. In the case of the LG devices described there, the (no-longer-valid) OpenType tables have been stripped from DroidSansArabic; we could, in principle, detect that and fall back to a different font that does support Arabic shaping (either shipped with Firefox or downloaded on demand). Here, however, the font *does* have OpenType tables, and those tables claim support for the Arabic script, so it'll be harder for us to detect the brokenness and trigger a fallback. I foresee some ugly hackery if we're going to work around this.... (sigh).
Comment 15 Jonathan Kew (:jfkthame) 2012-01-04 08:44:58 PST
Digging a bit further.....it turns out that the copy of DroidSansArabic from your device has several oddities:

- while the OpenType tables may not be quite as broken as I first thought - I was somewhat misled by FontForge, I think - they do seem to include references to glyphs that have been stripped out of the font, and this is probably why harfbuzz rejects them.

- the font contains a TSIV table, which is textual source code from Microsoft's VOLT tool, and should have been removed before shipping the font - it just bloats the file.

- the font also contains 'morx', 'prop' and 'just' tables, which are specific to Apple's AAT technology; unless these are supported on the Android device (which seems highly unlikely), they also represent pointless bloat.

- the font does not conform to TrueType/OpenType recommendations (it's unclear just how "normative" this requirement is) regarding the first four glyphs, which are expected to be .notdef, .null, CR and space. In particular, the absence of a space character may be confusing our code.

Looking at the VOLT source in the TSIV table, there are references to many glyphs that are not present in the font; and the glyph IDs mentioned in the GDEF table for Arabic glyphs that _are_ present do not match the actual glyph IDs in the font. What I currently suspect is that the OpenType tables were compiled (with VOLT) against a "complete" font, and then _subsequently_ some "subsetting" tool was used to remove non-Arabic glyphs; this left the font with OpenType tables that do not match the final glyph repertoire.
Comment 16 Jonathan Kew (:jfkthame) 2012-04-24 01:06:13 PDT
We now have support for presentation-forms shaping in harfbuzz. However, this will only take effect for fonts that lack a GSUB table.

In the case of this particular version of Droid Sans Arabic, the problem is that it *does* have a GSUB table (and so we still use the OpenType shaping path), but that table is completely incorrect for the glyphs actually present in the font. And so no useful shaping happens.

I've pushed a tryserver build at https://tbpl.mozilla.org/?tree=Try&rev=1784e001fcad that includes a patch intended to detect this particular version of the font and disregard its GSUB table, so that we'll use fallback shaping instead. Could you please try this build on the device with this bad font, and let me know whether it actually works? Thanks.
Comment 17 Jonathan Kew (:jfkthame) 2012-04-24 01:38:27 PDT
Created attachment 617820 [details] [diff] [review]
patch, ignore GSUB in broken version of Droid Sans Arabic

This attempts to work around the problem by detecting this specific broken version of Droid Sans Arabic, and ignoring its GSUB table. The font checksum in the 'head' table is used to recognize this specific font and avoid affecting other versions.

If we find that there are lots of slightly different but similarly broken versions of the font in circulation, we might want to consider unconditionally disabling GSUB for Droid Sans Arabic, and relying solely on presentation-forms shaping. I dislike that on principle, as presentation-forms shaping is a bad solution for Arabic-script rendering, but in practice it would probably be OK as this font is unlikely to use any more complex features.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 08:19:09 PDT
I tested the try server build Jonathan posted in comment 16, and it works great for me!
Comment 19 Jonathan Kew (:jfkthame) 2012-04-24 08:21:12 PDT
Comment on attachment 617820 [details] [diff] [review]
patch, ignore GSUB in broken version of Droid Sans Arabic

Ehsan reports that this fixed Arabic/Persian rendering on his device with the broken font. I think this is a sufficiently low risk that we should take it, or something along these lines, for the Android product ASAP.
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 14:55:37 PDT
I actually faced something which looks to be a regression in this build.  While using Google reader, sometimes the first line of the collapsed items which is displayed in a non-bold font disappears when you interact with the app.  It doesn't reproduce reliably for me, but usually I see this problem when I open a few posts and pan around.
Comment 21 John Daggett (:jtd) 2012-04-24 16:40:50 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> I actually faced something which looks to be a regression in this build.

Which build?  With the harfbuzz update applied or with the patch attached here?
Comment 22 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 16:52:43 PDT
(In reply to John Daggett (:jtd) from comment #21)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > I actually faced something which looks to be a regression in this build.
> 
> Which build?  With the harfbuzz update applied or with the patch attached
> here?

The try server build in comment 16.
Comment 23 John Daggett (:jtd) 2012-04-24 18:25:37 PDT
Comment on attachment 617820 [details] [diff] [review]
patch, ignore GSUB in broken version of Droid Sans Arabic

This is unfortunate but it can't be helped I guess.
Comment 24 John Daggett (:jtd) 2012-04-24 18:28:49 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> If we find that there are lots of slightly different but similarly broken
> versions of the font in circulation, we might want to consider
> unconditionally disabling GSUB for Droid Sans Arabic, and relying solely on
> presentation-forms shaping. I dislike that on principle, as
> presentation-forms shaping is a bad solution for Arabic-script rendering,
> but in practice it would probably be OK as this font is unlikely to use any
> more complex features.

I think if we find enough broken versions of this font we should simply ship a sane version of the font with Firefox and not add more hacky font logic.  We don't have logic to load Firefox-specific fonts currently but I think that's relatively easy to do and the MathML folks seem to be aiming for something effectively close to that.
Comment 25 Jonathan Kew (:jfkthame) 2012-04-24 23:49:58 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> I actually faced something which looks to be a regression in this build. 
> While using Google reader, sometimes the first line of the collapsed items
> which is displayed in a non-bold font disappears when you interact with the
> app.  It doesn't reproduce reliably for me, but usually I see this problem
> when I open a few posts and pan around.

Strange. I don't see how this patch could cause something like that - maybe an existing erratic issue? Could you try to reproduce with a recent nightly that doesn't have the arabic fix?

Was this specifically when reading Arabic/Persian stuff in GR, or do you mean you saw issues on *non-Arabic* pages when using the try build?
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 20:03:04 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #25)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > I actually faced something which looks to be a regression in this build. 
> > While using Google reader, sometimes the first line of the collapsed items
> > which is displayed in a non-bold font disappears when you interact with the
> > app.  It doesn't reproduce reliably for me, but usually I see this problem
> > when I open a few posts and pan around.
> 
> Strange. I don't see how this patch could cause something like that - maybe
> an existing erratic issue? Could you try to reproduce with a recent nightly
> that doesn't have the arabic fix?

I have not seen that issue in any other Fennec build.

> Was this specifically when reading Arabic/Persian stuff in GR, or do you
> mean you saw issues on *non-Arabic* pages when using the try build?

No, this was an issue when reading English content on Google Reader (Planet Mozilla to be precise.)

But yeah maybe this was a problem unrelated to this?  If you can make a new try server build based on a more recent m-c revision, I'd be happy to test it.
Comment 27 Jonathan Kew (:jfkthame) 2012-04-26 03:17:41 PDT
Could you try https://tbpl.mozilla.org/?tree=Try&rev=79ade3199eab and see if that shows problems? (Based on mozilla-inbound tip  as of this morning.)
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-26 12:00:00 PDT
This try build doesn't have that bug, so your patch is not to blame here.
Comment 30 Ed Morley [:emorley] 2012-04-27 07:06:29 PDT
https://hg.mozilla.org/mozilla-central/rev/42ef0ce00b8c
Comment 31 Jonathan Kew (:jfkthame) 2012-05-03 12:30:05 PDT
Comment on attachment 617820 [details] [diff] [review]
patch, ignore GSUB in broken version of Droid Sans Arabic

[Approval Request Comment]
Regression caused by (bug #): broken font on some Android devices

User impact if declined: Arabic text unreadable on affected devices

Testing completed (on m-c, etc.): since this landed on m-c, the issue is fixed in Nightly (as confirmed by Ehsan)

Risk to taking this patch (and alternatives if risky): minimal risk - mobile-only change, narrowly targeted to only affect the one known-broken font

String changes made by this patch: none

If we uplift this to Aurora, does that mean it'll go into the native-fennec beta release? Given the impact for users with affected devices, and the fact that the patch carries essentially no risk to anything else - it specifically targets just one version of one font - I think we should fix this for the beta if at all possible.
Comment 32 Alex Keybl [:akeybl] 2012-05-06 19:19:11 PDT
Comment on attachment 617820 [details] [diff] [review]
patch, ignore GSUB in broken version of Droid Sans Arabic

[Triage Comment]
Given the narrow scope and high reward, approving for Aurora 14.
Comment 33 Jonathan Kew (:jfkthame) 2012-05-07 01:41:05 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/37b8a2df105d

Note You need to log in before you can comment on or make changes to this bug.