Closed Bug 534919 Opened 16 years ago Closed 16 years ago

Incorrect font and broken Arabic shaping for the letter after ZWNJ for pages with lang=fa/ar specifying a non-existing font and a generic fallback

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- alpha1+
status1.9.2 --- final-fixed

People

(Reporter: ehsan.akhgari, Assigned: jfkthame)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 2 obsolete files)

Testcase
534 bytes, text/html
Details
patch v2.1: fixed the reftest so it actually passes
3.63 KB, patch
Details | Diff | Splinter Review
Attached file Testcase —
The following conditions on a page cause the following problems: 1. The page should have lang=fa or lang=ar on the <html> tag. 2. The page should specify a non-existing font and a generic fallback, such as: font-family:foobar,sans-serif. 3. A ZWNJ character should appear in the text. The problems caused by the above conditions: 1. Arabic shaping is broken for the letter after ZWNJ (i.e., the letter appears in isolated form, no matter what its following letter is.) 2. The font chosen for the letter after ZWNJ is incorrect. See the attached testcase for an example of this problem. This might be related to bug 534260...
Assignee: nobody → ehsan.akhgari
See Also: → 534260
Assignee: ehsan.akhgari → nobody
I'm not seeing this bug on 3.6 or trunk on Windows.
I'm seeing this on 3.6 with Mac as well. This is pretty serious for Arabic and all languages based on the Arabic script, and I think this is worth a blocking status, although I'm not sure if we want to block on it for 1.9.2 at this stage, but there isn't currently any way to nominate this to block 1.9.2.x.
Severity: normal → major
blocking2.0: --- → ?
Flags: wanted1.9.2?
Flags: blocking1.9.2?
I think I know what's happening here. It's not really a regression; however, it is possible (depending on your installed fonts) that you'll see it more often on OS X trunk (using Core Text) than on earlier releases (using ATSUI). The reason for this is that Core Text supports Arabic script with OpenType fonts as well as ATSUI, and therefore font fallback may make different choices. The issue is that the default Arabic font (Geeza Pro), which is what you'll see with the CSS in your example, does not support the ZWNJ character. Therefore, when we do font matching before doing the text layout, we search for another font that does support ZWNJ. So far so good. However, the font matching in gfxFontGroup::FindFontForChar has special code for join-control characters: in particular, it tries to use the same font for a following letter as for the joiner. Here's the problem. If FindFontForChar finds some other, non-Arabic font for the ZWNJ, and then find that this font doesn't support the following Arabic letter, all is still OK. This is what probably happens on a default OS X / FF3.5 (ATSUI) setup. The character after the ZWNJ goes back to the default fallback path, and gets Geeza Pro (our preferred Arabic font). But if the font that is chosen for the ZWNJ *does* include Arabic letters as well, then things go awry: because of the code at http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxFont.cpp#1569, the following letter(s) will inherit that font, rather than reverting to what was in effect before the ZWNJ. And this situation becomes more likely once Core Text is enabled (trunk builds), because this allows us to use OpenType fonts such as Code2000 for Arabic, which we would reject (because of lack of AAT support) in the ATSUI-only world. (Note that in principle, this issue can occur on Windows as well, if you set the default Arabic font to something that *doesn't* support ZWNJ. But IIRC, the standard fonts like Times New Roman do support it, and so it's unlikely people will run into this.)
(In reply to comment #2) > I'm seeing this on 3.6 with Mac as well. FWIW, I can repro in 3.5 on Mac as well, because I have an AAT Arabic font that supports ZWNJ, and fallback on the ZWNJ picks that font. So then the following char(s) continue to fall back to that font instead of Geeza. Does this happen with only Apple system fonts enabled, or are additional fonts needed in order for the situation to occur?
Looking at the code in gfxFontGroup::FindFontForChar, I'm not sure why we care about trying to match fonts with adjacent *non*joiners. It makes sense for ZWJ, because if we don't have that in the same font run as the adjacent characters, it can't have its expected effect on shaping. But ZWNJ and WJ (U+2060) are supposed to break cursive connection anyway, so I don't see a need to special-case them like this. I'll prepare a patch, and try reftesting to see if it has effects I haven't foreseen; then we should get jdaggett to review, as I think he wrote this stuff.
(In reply to comment #5) > Looking at the code in gfxFontGroup::FindFontForChar, I'm not sure why we care > about trying to match fonts with adjacent *non*joiners. It makes sense for ZWJ, > because if we don't have that in the same font run as the adjacent characters, > it can't have its expected effect on shaping. But ZWNJ and WJ (U+2060) are > supposed to break cursive connection anyway, so I don't see a need to > special-case them like this. I agree with your reasoning here, in fact I was just commenting on why do things this way. let me know if you need help testing your patch.
(In reply to comment #4) > (In reply to comment #2) > > I'm seeing this on 3.6 with Mac as well. > > FWIW, I can repro in 3.5 on Mac as well, because I have an AAT Arabic font that > supports ZWNJ, and fallback on the ZWNJ picks that font. So then the following > char(s) continue to fall back to that font instead of Geeza. > > Does this happen with only Apple system fonts enabled, or are additional fonts > needed in order for the situation to occur? I have not installed any fonts explicitly on my machine, however I'm not sure if some of the software that I run has installed their own fonts. I will be happy to test this if you let me know how can I make sure that only Apple system fonts are enabled.
Just starting a build; if all goes well, I'll post the patch here and push it to tryserver. To test with minimal fonts, you could (temporarily) remove any font files from ~/Library/Fonts. It's also possible that some packages may have installed fonts into /Library/Fonts, but most of what's there probably comes from the OS distribution. Then it's also possible to activate fonts that are not in the standard folders, so you could look through Font Book and check whether others are showing up there. Or you could get the Apple Font Tools package (see under Text & Fonts on developer.apple.com) and use the ftxinstalledfonts tool to check the paths of all active fonts. (Overall, I guess it's not all that easy to know what's "original" from the OS and what's been added. Hmm... wonder if someone maintains a list someplace.)
Checking some fonts with Character Viewer, there are definitely some Apple-supplied Arabic fonts that include ZWNJ, and Geeza Pro does not, so there is at least the possibility of this problem showing up on a default configuration. Taking a guess here: try using Font Book to disable the Al Bayan family, and see if that makes a difference.
(In reply to comment #9) > Checking some fonts with Character Viewer, there are definitely some > Apple-supplied Arabic fonts that include ZWNJ, and Geeza Pro does not, so there > is at least the possibility of this problem showing up on a default > configuration. Taking a guess here: try using Font Book to disable the Al Bayan > family, and see if that makes a difference. Disabling the Al Bayan family mitigates the problem.
(In reply to comment #8) > Just starting a build; if all goes well, I'll post the patch here and push it > to tryserver. > > To test with minimal fonts, you could (temporarily) remove any font files from > ~/Library/Fonts. It's also possible that some packages may have installed fonts > into /Library/Fonts, but most of what's there probably comes from the OS > distribution. ~/Library/Fonts is empty on my system. > Then it's also possible to activate fonts that are not in the standard folders, > so you could look through Font Book and check whether others are showing up > there. Or you could get the Apple Font Tools package (see under Text & Fonts on > developer.apple.com) and use the ftxinstalledfonts tool to check the paths of > all active fonts. (Overall, I guess it's not all that easy to know what's > "original" from the OS and what's been added. Hmm... wonder if someone > maintains a list someplace.) Well, since I have no idea what the default font set on Mac is, I don't think this is going to reveal anything for us here.
(In reply to comment #10) > Disabling the Al Bayan family mitigates the problem. Ok, that matches what I see. Fallback is choosing Al Bayan for the ZWNJ, and then adjacent-char propagation is causing us to use this for the following letter as well, while the rest of the text is Geeza Pro.
(In reply to comment #12) > Created an attachment (id=417797) [details] > patch v1: only special-case ZWJ (not non-joiners) in font-selection code Hmm, I think this just narrows down the scope of the problem. What if we use &zwj; instead of &zwnj; in the test case? I mean, what if a certain Arabic font doesn't include ZWJ? With this patch, still cases like |foo&zwj;bar| would cause things to render incorrectly if the text is being rendered in a font which doesn't include ZWJ. I think the ideal thing to happen here is that if a font doesn't include ZWJ, we should just grab that character from a fallback font and render all the other characters in the specified font (if they all exist in that font, of course.) Don't you agree?
Attachment #417797 - Flags: review?(roc)
(In reply to comment #14) > (In reply to comment #12) > > Created an attachment (id=417797) [details] [details] > > patch v1: only special-case ZWJ (not non-joiners) in font-selection code > > Hmm, I think this just narrows down the scope of the problem. What if we use > &zwj; instead of &zwnj; in the test case? I mean, what if a certain Arabic > font doesn't include ZWJ? With this patch, still cases like |foo&zwj;bar| > would cause things to render incorrectly if the text is being rendered in a > font which doesn't include ZWJ. That's a much less likely scenario, though; Geeza Pro (for example - the specific example where this happens!) *does* include ZWJ, as it must if it's to shape properly. > > I think the ideal thing to happen here is that if a font doesn't include ZWJ, > we should just grab that character from a fallback font and render all the > other characters in the specified font (if they all exist in that font, of > course.) > > Don't you agree? We could consider that, but the rendering still wouldn't be right (ZWJ would not cause joining forms to be chosen), so I don't think it gains us much - arguably it'd be better to get the right form but the wrong font than the wrong form from the right font. In any case, it seems unlikely to arise, as ZWJ is present in the default Arabic font we're using (and probably in any others we're likely to use elsewhere or in the future).
Comment on attachment 417797 [details] [diff] [review] patch v1: only special-case ZWJ (not non-joiners) in font-selection code Need a reftest here.
Attachment #417797 - Flags: review?(roc) → review+
A reftest won't be completely reliable, as it depends on installed fonts and fallback search. But I can do one that fails (without the patch) on a default OS X system, I believe.
(In reply to comment #17) > A reftest won't be completely reliable, as it depends on installed fonts and > fallback search. But I can do one that fails (without the patch) on a default > OS X system, I believe. Can't we create a reftest using a custom web font?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2+
Assignee: nobody → jfkthame
Attachment #417797 - Attachment is obsolete: true
Attachment #417817 - Flags: review?(roc)
Oops, reftest could fail spuriously if the zwnj fallback affected line spacing. Explicitly set a large line height to avoid this.
Attachment #417817 - Attachment is obsolete: true
I'm leaving for today, and the tree is not green yet... I'll land this tomorrow if Jonathan doesn't beat me to it.
http://hg.mozilla.org/mozilla-central/rev/7210af2c4368
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee: jfkthame → ehsan.akhgari
Whiteboard: [needs 1.9.2 landing]
Assignee: ehsan.akhgari → jfkthame
Pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6f1a7758de3c
status1.9.2: --- → final-fixed
Whiteboard: [needs 1.9.2 landing]
Marking the 14 bugs that are both: * nominated for blocking1.9.3:? * fixed on the 1.9.2 branch (according to status1.9.2) as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually. They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.
blocking2.0: ? → alpha1
Keywords: regressionwindow-wanted
Filed bug 705044 on the reftest failing on Mac OS X 10.7 (Lion).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: