986802 - The "lam-alif" arabic character It does not appear correctly with using letter-spacing

Status: RESOLVED FIXED

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

Description

[blinky]

The "lam-alif" character It does not appear correctly on Windows XP with using letter-spacing.

Testcase: https://dl.dropboxusercontent.com/u/95157096/85f61cf7/c7ep2n4P.html

Screenshot: https://dl.dropboxusercontent.com/u/95157096/85f61cf7/TnA5z6tP.png

Comment 1

[blinky]

This bug affects the titles in YouTube.

See https://dl.dropboxusercontent.com/u/95157096/85f61cf7/fe6z6jovs.png

Comment 2

[blinky]

This problem occurs since the version 13.0a1 (2012-02-16).

Screenshot: https://dl.dropboxusercontent.com/u/95157096/85f61cf7/44zu5nwef.png

http://hg.mozilla.org/mozilla-central/rev/a853f4017192
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120216 Firefox/13.0a1
Build ID: 20120216031231

Comment 3

[Alice0775 White]

works for me on Firefox ESR24.4.0 and 28+ Windows7.

However, I can reproduce the problem on WinXP.


Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=91d77c934b26&tochange=392319d8c1fa
Triggered by:
392319d8c1fa	Jonathan Kew — bug 695857 - update harfbuzz code and adapt gfx/thebes for revised harfbuzz API. r=jdaggett harfbuzz library code from upstream harfbuzz-ng commit f51e167436a51b890ffe3f7f7920498fa287acd9; replaced HB_SCRIPT_* with MOZ_SCRIPT_* codes in gfx; adapted for changes to HB function signatures.

Comment 4

[Jonathan Kew [:jfkthame]]

cc'ing Behdad, as this is really a harfbuzz issue.

The problem here arises because of how the Arabic features in arial.ttf on WinXP have been constructed. The lam-alef ligature is handled by a lookup that is used by two features, 'rlig' and 'liga'. However, the 'rlig' feature is not included in the normal list of lookups for the 'arab' script, it is only referenced by the requiredFeature field.

In hb_ot_map_builder_t::compile, the lookups for the requiredFeature are added to the map as an initial step, before the remaining features that the plan has requested. But in this position, the Arial 'rlig' lookup won't work, because it expects to see glyphs that have already been shaped by 'init', etc., not the nominal glyphs from the 'cmap'.

In normal use, the lam-alef ligature still works, because the 'liga' feature adds that lookup again later in the sequence. But when letter-spacing is in effect, and Firefox therefore disables 'liga', we're left with the 'rlig' lookup at the beginning of the lookup list, and it fails.

With the Win8 version of Arial, this problem doesn't arise, because 'rlig' is included in the standard collection of features for the script, and so collect_features_arabic() inserts it after the basic joining forms have already been processed.

I've confirmed that if I move the handling of the requiredFeature in hb_ot_map_builder_t::compile() such that its lookups are appended to the last stage of feature processing, instead of inserted at the beginning, then the lam-alef ligature in WinXP Arial still works even with 'liga' disabled. However, this is not really a satisfactory fix; it's a purely arbitrary hack that could equally well break the processing of required features in other fonts.

The trouble, really, is that the various specs (the general OpenType spec and the various script-shaping specs) have conflicting requirements re the order of processing lookups; and once we depart from the generic OT model whereby they're simply processed in the order in which the lookups appear in the font, there's no spec that tells us where to put the requiredFeature's lookups in relation to the ordered sequence of features that some of the shapers apply.

The only sensible way forward I can think of at the moment is to check what the feature tag of the requiredFeature is, and if it's known to the shaper (like 'rlig'), then insert its lookups exactly as if that feature had been specified in the script/langsys feature list, even if it is not actually present there.

This will still leave us unsure what to do if a font has requiredFeature pointing at an arbitrary feature that is unknown to the shaper; clearly, we should still apply its lookups, but we don't know when to do so. But that's a sufficiently obscure case that I suggest we ignore it, and arbitrarily decree that if the requiredFeature's tag is unknown, it will be processed wherever it is most convenient for us to put it - probably after all specifically-ordered shaper features, together with any random user features that are being added.

Comment 5

[Behdad Esfahbod]

Thanks Jonathan.  Commented on the github pull-request.  Copying here:

Thanks Jonathan.

Three points:

  1. I'd add the new feature_tag argument after the existing feature_index.  I've done that here: https://github.com/behdad/harfbuzz/commit/935abef1466ae979cc3c4aa6219e16012fc8adbf

  2. Given that 'locl' feature is applied in first stage, I think applying RequiredFeature *after* everything else (ie. in the last stage) is most useful.

  3. What I don't like about your approach is that it disables some lookups that might have been enabled previously.  Eg. if the font designer sets a RequiredFeature that happens to be called 'liga', then we won't apply the normal 'liga' anymore even though we should...

I think I really prefer to just move the RequiredFeature application to the last stage.  Your approach is quite neat, but makes too much assumptions on the tag of the feature being set correctly.

I'm undecided, please advise which way you think we should take this.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Behdad Esfahbod from comment #5)

>   2. Given that 'locl' feature is applied in first stage, I think applying
> RequiredFeature *after* everything else (ie. in the last stage) is most
> useful.

For a recognized tag, I think we should apply at the same stage it would have been applied if it were in the main feature list, which is the main goal of the patch here.

For an unknown RequiredFeature, only an analysis of fonts that actually have one (if there are any) could tell us what the designers expected. In the absence of actual examples, and given that the spec says this feature is "required" and all others are "optional", it seems reasonable to suppose that the RequiredFeature should be applied first, and then the remainder.

>   3. What I don't like about your approach is that it disables some lookups
> that might have been enabled previously.  Eg. if the font designer sets a
> RequiredFeature that happens to be called 'liga', then we won't apply the
> normal 'liga' anymore even though we should...

This is part of the broader question of whether user features should be able to override (disable) the RequiredFeature or not. With the WinXP Arial font, where both the RequiredFeature 'rlig' and the normal 'liga' feature point to the same lookup, my approach means that

  --features liga=0

does not disable lam-alef, because rlig still applies; and

  --features rlig=0

does not disable it either, because liga still applies; but

  --features liga=0,rlig=0

*will* disable it. I'm inclined to think this is OK: it's not really different, from a user's point of view, from the fact that

  --features init=0

will disable a feature that is "required" for correct rendering of the script.
> 
> I think I really prefer to just move the RequiredFeature application to the
> last stage.  Your approach is quite neat, but makes too much assumptions on
> the tag of the feature being set correctly.
> 
> I'm undecided, please advise which way you think we should take this.

The single RequiredFeature index is an obsolete mechanism that isn't widely used, but when it does occur, I think we should basically treat it as though it were part of the langSys's feature list. The only special consideration arises if it's a feature we wouldn't normally apply by default; then because it's the RequiredFeature, we do apply it.

I did consider the option of simply moving the RequiredFeature to the last stage (see middle of comment 4), but although that fixes the WinXP Arial issue here, it seems much more arbitrary than the approach I chose.

So....having tried to re-think it all again, I'm still inclined this way.

One possible modification would be to make the RequiredFeature "always-on", so that we'd ignore a user feature setting that tries to disable it. But I don't currently see any strong reason to do this, and it's debatable whether it would be a better or worse design anyhow.

Comment 7

[Behdad Esfahbod]

I think you missed my biggest objection.  What I was saying is this:

Imagine script 'arab' langsys 'FAR ' has RequiredFeature index 0, and one feature with index 1.  Both feature 0 and feature 1 have tag 'rlig'.  Previously, we were applying both features 0 and 1.  With your patch, we will be only applying feature 0.

Here's my proposal: if the RequiredFeature's tag matches any tag known to the shaper, use that tag to decide which stage to apply RequiredFeature in.  Otherwise, apply in last stage.

WDYT?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Behdad Esfahbod from comment #7)
> I think you missed my biggest objection.  What I was saying is this:
> 
> Imagine script 'arab' langsys 'FAR ' has RequiredFeature index 0, and one
> feature with index 1.  Both feature 0 and feature 1 have tag 'rlig'. 
> Previously, we were applying both features 0 and 1.  With your patch, we
> will be only applying feature 0.

Oh, I see - yes, I didn't follow what you meant there.

But we already have that "problem", in a way. Imagine a font where script 'arab', langsys 'FAR ' has no RequiredFeature; FeatureCount = 2; FeatureIndex[0] = 0 and FeatureIndex[1] = 1. Both feature 0 and feature 1 have tag 'rlig'. What will we do? AFAICS, we'll only apply one of them, because we'll use hb_ot_layout_language_find_feature to look up the feature index of 'rlig', and that will return only one index, not both. 

In either case, I think the font is violating the intent (if not the letter) of the spec, which assumes there will be a single FeatureRecord for any given feature tag in a particular script/langsys.

> Here's my proposal: if the RequiredFeature's tag matches any tag known to
> the shaper, use that tag to decide which stage to apply RequiredFeature in. 

My proposed patch does that, right?

It *also* makes the RequiredFeature take priority over any other feature in the list that has the same tag. I don't think that's a problem, because I don't think fonts should have repeated feature tags that apply at the same time, but if you really want to rework it to avoid this, I guess it's fine. I doubt it'll make any difference in practice.

> Otherwise, apply in last stage.

I think applying in first stage makes more sense for an "unknown required feature", but I don't really care - there may well not be any real fonts out there that will be affected anyhow. Until we run across an example, it's a pretty arbitrary choice.

Comment 9

[Behdad Esfahbod]

Humm, my WinXP arial.ttf doesn't have any required features.

Comment 10

[Behdad Esfahbod]

Oops.  My bad.

Comment 11

[Behdad Esfahbod]

Ok I did a quick survey of all fonts I had access to, and most fonts that have a required-feature have it as feature index 0.  The exceptions are all WinXP fonts: arialuni, micross, timesbd, arial, palab, arialbd, pala, times, palai, palabi.

The ones that have it as feature 0 *all* have feature tag " RQD".  I suppose that's a FontForge thing.  They are a handful of Thai fonts, DejaVu, and FreeFonts.  All are safe no matter which stage we apply since they all go through single-stage shapers.

Re the XP fonts:

- Arial and Times family have the same tables, which maps Required to 'rlig' and will be fine as long as we apply it at 'rlig' stage or last stage,

- arialuni (both on XP and on OS X) has Required feature index 8 *for GPOS* of knda.  So it doesn't matter.  But curiously, it has a tag 'requ'.

- Palatino family have required index 9 for Romanian, that has tag 'liga'.  That langsys also has a 'liga', but the 'liga' feature is a subset of the required feature.  So I suppose, ideally, turning 'liga' off should NOT disable anything for this langsys.


So, looks like either of fixes we have suggested work for these all.  What I'm most uncomfortable with is that the feature tag of the required-feature is unused according to the spec...

Still, I'm happy to modify your approach to what I suggested in comment 7 and go with that.  Will try to do today.

Comment 12

[Behdad Esfahbod]

Jonathan, can you please review / test this:

https://github.com/behdad/harfbuzz/commit/59080469211f296453d7948c0990fe2d50a55a7a

Thanks.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Behdad Esfahbod from comment #12)
> Jonathan, can you please review / test this:
> 
> https://github.com/behdad/harfbuzz/commit/
> 59080469211f296453d7948c0990fe2d50a55a7a
> 
> Thanks.

Looks OK to me, thanks. Once you merge it, we'll pull a new copy of harfbuzz and get this out in Nightly builds.

Comment 14

[Behdad Esfahbod]

Ugh, I thought I fixed this.  Anyway, should be fixed now:

commit da132937989acb4d8ca9bd41c79f98750e7dda30
Author: Jonathan Kew <[e-mail]>
Date:   Sun Apr 27 14:05:24 2014 +0100

    Rework handling of requiredFeature to solve problem with rlig in arial.ttf from winxp
    
    https://bugzilla.mozilla.org/show_bug.cgi?id=986802
    Fixes https://github.com/behdad/harfbuzz/pull/39
    
    API Change:
    
    -hb_ot_layout_language_get_required_feature_index
    +hb_ot_layout_language_get_required_feature
    
    New API takes an extra pointer argument.  Pass NULL in to get
    behavior of previous API.
    
    Reworked by behdad

Comment 15

[blinky]

This bug has been fixed in bug 1036981.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to blinky from comment #15)
> This bug has been fixed in bug 1036981.

Correct; it was fixed in upstream harfbuzz (comment 14), and we updated our copy in bug 1036981, so we now have the fix in gecko.

-> Resolved:Fixed.