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

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

Attachment: 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)

Comment 1

[Simon Montagu :smontagu]

Attachment: screenshot

Comment 2

[Simon Montagu :smontagu]

Attachment: Screenshot on OSX

The previous screenshot was on Ubuntu Linux. This one is on SeaLeopard, and also shows regressions, though much less serious ones.

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: patch, force non-spacing diacritics to be zero width

This is the patch from bug 635639, updated to apply to current trunk code.

Comment 7

[Jonathan Kew [:jfkthame]]

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.

Comment 8

[Simon Montagu :smontagu]

Attachment: Screenshot with the patch and updated fonts

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?

Comment 9

[Jonathan Kew [:jfkthame]]

(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.

Comment 10

[Behdad Esfahbod]

(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.

Comment 11

[Jonathan Kew [:jfkthame]]

(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 12

[Behdad Esfahbod]

Comment on attachment 597782 [details] [diff] [review]
patch, force non-spacing diacritics to be zero width

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

LGTM.

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cddfe9097c33

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cddfe9097c33