Closed Bug 727736 Opened 8 years ago Closed 8 years ago

Diacritic placement in RTL scripts not working well in many fonts since the last harfbuzz update

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: smontagu, Assigned: jfkthame)

References

Details

(Keywords: regression, rtl)

Attachments

(5 files)

Attached file Testcase
Many fonts have misplaced diacritics in RTL scripts since the last harfbuzz update in bug 695857. Compare the rendering of the testcase with harfbuzz turned on and off for Hebrew and Arabic (gfx.font_rendering.harfbuzz.scripts set to 1 and 7)
Attached image screenshot
Attached image Screenshot on OSX
The previous screenshot was on Ubuntu Linux. This one is on SeaLeopard, and also shows regressions, though much less serious ones.
Your Linux screenshot is puzzling; AIUI, you've got Arial 3.00 and Courier New 2.90 there, which are the versions from WinXP. But on a WinXP VM, with these same font versions, I don't see the same spacing problems. The Arabic diacritics (especially kasra) are poorly placed, but that's just because the fonts lack mark-positioning support, and occurs with either harfbuzz or uniscribe rendering.
Trying the XP versions of Arial and Courier New on OS X, I see the spaced-out rendering of Hebrew under both Harfbuzz *and* Core Text shaping. The Arabic, however, only breaks up under Harfbuzz; Core Text suppresses the diacritic widths.
The "diacritics cause gaps in the text" issue seems to be basically another version of bug 635639, though it's odd that the reftests there are not failing - the exact details of the fonts involved must be slightly different.

Behdad, it looks like the patch from bug 635639 is not present in current harfbuzz trunk. Is this deliberate, or was it an oversight? After rebasing that patch to the current code and applying it here, the spacing problem with the old Arial and Courier New fonts is resolved, so it looks to me like we still need that code.
This is the patch from bug 635639, updated to apply to current trunk code.
Attachment #597782 - Flags: review?(mozilla)
Incidentally, when testing changes in gfx.font_rendering.harfbuzz.scripts, you may notice that currently-displayed text is not updated; you have to restart the browser, or at least zoom to a different size to see the effect of the change (and zooming back still shows the old rendering, unless you wait long enough for caches to expire). Filed bug 727815 about this annoyance.
OK, do you want the good news or the bad news? With attachment 597782 [details] [diff] [review] and the same fonts, I don't see the spaced-out-text effect any longer. I do see the regressions with multiple diacritics on one base character in Hebrew, as in attachment 597710 [details].

However, with version 5.11 of Courier (from Windows 7), I see the spaced-out bug in a different form: each diacritic is correctly placed on its base character, but there are extra empty spaces between the characters. That doesn't seem to be a regression from the last harfbuzz update though, it happens in Firefox 10 already.

The patch also doesn't affect the regressions described in bug 662055 comment 16 and bug 721821 comment 9. Would you like me to open separate bugs for all those cases?
(In reply to Simon Montagu from comment #8)

> However, with version 5.11 of Courier (from Windows 7), I see the spaced-out
> bug in a different form: each diacritic is correctly placed on its base
> character, but there are extra empty spaces between the characters. That
> doesn't seem to be a regression from the last harfbuzz update though, it
> happens in Firefox 10 already.

That sounds like something we should file and investigate separately. Does it happen on other platforms (with the same font), or is it Linux-specific?

> The patch also doesn't affect the regressions described in bug 662055
> comment 16

I've just posted a patch to bug 662055 which should address this.

> and bug 721821 comment 9. Would you like me to open separate bugs
> for all those cases?

Let's see what happens with bug 721821; I haven't tried to dig into that one yet, but we may be able to deal with all the SVG-text aspects as a single issue.
(In reply to Jonathan Kew (:jfkthame) from comment #5)

> Behdad, it looks like the patch from bug 635639 is not present in current
> harfbuzz trunk. Is this deliberate, or was it an oversight? After rebasing
> that patch to the current code and applying it here, the spacing problem
> with the old Arial and Courier New fonts is resolved, so it looks to me like
> we still need that code.

I wanted to do it properly (synthesize GDEF), but obviously have not got to yet.  Will reprioritize.
(In reply to Behdad Esfahbod from comment #10)
> I wanted to do it properly (synthesize GDEF), but obviously have not got to
> yet.  Will reprioritize.

Thanks - do you see any problem with us using the rebased patch from 635639 (as attached here) in the meantime, at least in our tree, to fix the regression for current nightlies?
Comment on attachment 597782 [details] [diff] [review]
patch, force non-spacing diacritics to be zero width

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

LGTM.
Attachment #597782 - Flags: review?(mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cddfe9097c33
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/cddfe9097c33
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.