[designspaceLib] [varLib] Allow FeatureVariations to be processed *after* other features

[justvanrossum]

This PR implements #1625 and resolves #1371.

1. addFeatureVariations(): allow the feature tag to be specified (default to 'rvrn')
2. allow said feature to already exist, in which case we append new lookup indices to the existing feature

Item 2 needs a test case.

Update: what follows has been implemented in the meantime.

This needs integration with varLib._add_GSUB_feature_variations(), and some way in the designspace file to specify what to do.

I propose the following:
Add a processing attribute to the <rules> tag, which can have a value of "pre" or "post". "pre" will be default, and will use the rvrn feature for FeatureVariations. "post" will use the rclt feature instead, and will cause the substitutions to be done last in the GSUB processing order.

Example:

<?xml version='1.0' encoding='utf-8'?>
<designspace format="?.?">
    <rules processing="post">
       <rule ...>
    </rules>
    ...
</designspace>

I suggest to do designspace integration in a separate PR. Integration with designspaceLib and varLib has also been implemented in this PR.

[anthrotype]

Thanks Just! I'll take a look tomorrow.

[justvanrossum]

Something I just realized that I'm not sure how to address: what if there exists multiple features with <featureTag>?

[behdad]

Something I just realized that I'm not sure how to address: what if there exists multiple features with ?

You should update all. Or make the function alternatively take a feature index / set of feature indices and update those, or additionally take script and language tags (and default to updating all).

[justvanrossum]

Right, I think I understand. Where it currently does this:

        record = buildFeatureTableSubstitutionRecord(varFeatureIndex, existingLookupIndices + lookupIndices)
        featureVariationRecords.append(buildFeatureVariationRecord(conditionTable, [record]))

...it should build a record for each existing feature.

[justvanrossum]

You should update all.

Done in db04262.

[justvanrossum]

If we can agree on how to enhance the designspace file format, I can implement that here, too. Looks like it'll be fairly easy to do.

[justvanrossum]

Designspace file: perhaps <rules processing="first"> (default, same as <rules>) and <rules processing="last"> is better than "pre" and "post" as values.

But I'm open to suggestions to make this more clear.

I'm trying to avoid making this too OpenType-specific, and I want to keep the functionality simple. It is not my intention to implement all that is possible with feature variations, but to make the common case easy to specify. (In hindsight we should've never used 'rvrn' in the first place, but should have went straight to 'rclt'.)

[justvanrossum]

And in addition to a new <rules> attribute I propose a new attribute to the DesignSpaceDocument object, for example doc.rulesProcessingLast, which can be True or False, mapping to <rules processing="last"> if True and <rules processing="first"> (aka <rules>) if False.

(doc.rules is a simple list, and I wouldn't want to change that, and a new top-level doc attribute seems most appropriate to me.)

[anthrotype]

This is looking good, thanks.

I was only a bit uncertain about the use of rclt feature, since the spec defines this specifically for contextual alternates (GSUB Lookup 6), but I guess it works with any GSUB lookup type.
Basically, we are relying on the fact that rclt is always on (like rvrn), but it is not reordered first and is applied in the order of the lookup list, thus it can override previous ones.

I imagine that you tested that this works on all the major OSes and layout engines (I haven't had the chance yet).

Bonus point if we add a test for the case when rclt is already present.

[justvanrossum]

Wrt 'rclt', see this bit of twitter: https://twitter.com/justvanrossum/status/1184849808462405635?s=19

I have some test results with this approach, with help from @irenevl and @Lorp, which I have to dig up. There will be some caveats ('rclt' is relatively new, but so is 'rvrn') , but right now I am convinced it's the best we've got.

I'll work on a test case with existing 'rclt'.

[justvanrossum]

Regarding using 'rclt' instead of 'rvrn': I made a test font last year, and @irenevl was so kind to test it in several environments. Here findings on 14 december 2018 were as follows:

here is my report, some good news and some bad news :)

I'm testing on the Beta MacOS, so not sure if this is a buggy thing of the future :)

OS
10.14.3 Beta (18D21c)

Software

Safari: Version 12.0.3 (14606.4.2)
Yes!

Chrome: Version 71.0.3578.98
Yes!

Chrome Canary: Version 73.0.3640.0
No!

Firefox: Version 64.0
No! (+overlaps are showing)

Illustrator CC (2019): v 23.0.1
No!

Photoshop CC (2019): v 20.0.1
No! (It doesn't even appear in the menu)
that’s seems to be a different kind of bug though.

The test font from back then:

FeaVarTestFont-VF-rclt.ttf.zip

It only supports the character 'a', and has a 'smcp' feature, and an axis called TEST that switches a and a.smcp to a.feav and a.smcp.feav respectively, if all behaves well.

[justvanrossum]

Expected results:

$ hb-view FeaVarTestFont-VF-rclt.ttf --output-file="feavartest-plain.png" --font-size=100 --features="-smcp" --variations="TEST=200" "aaa"

$ hb-view FeaVarTestFont-VF-rclt.ttf --output-file="feavartest-smcp.png" --font-size=100 --features="smcp" --variations="TEST=200" "aaa"

$ hb-view FeaVarTestFont-VF-rclt.ttf --output-file="feavartest-plain-feav.png" --font-size=100 --features="-smcp" --variations="TEST=700" "aaa"

$ hb-view FeaVarTestFont-VF-rclt.ttf --output-file="feavartest-smcp-feav.png" --font-size=100 --features="smcp" --variations="TEST=700" "aaa"

[twardoch]

There is also:

• [fea syntax] OpenType Variation support? adobe-type-tools/afdko#153
• https://github.com/adobe-type-tools/afdko/labels/fea%20syntax%20extension

Are we sure that extending the rules designspace section possibly ad infinitum is the best course of action?

At some point we'll arrive at conditional contextual substitutions and conditional positioning.

This really should be done in the FEA syntax and feaLib, I think.

[twardoch]

Even if there are things that will not be implemented in one of the three FEA compilers (makeotf, feaLib and fontforge), I don't think we should consider the FEA syntax tabula rasa.

There are FEA syntax aspects that are not implemented in makeotf already, and there are aspects not implemented in feaLib and in fontforge. But that's the difference between spec and implementation.

I think that "rules" should remain a simple hack (above all: one that is easily implemented in both static and variable fonts, so limited to 1:1 glyph replacement, and ordering-agnostic).

Specifying feature tags in rules is, to me, scary. It starts looking like some kind of "patcher" for FEA that exists only because FEA currently cannot express it.

I'm not even sure if the processing=post is a good idea. How would you implement processing=pre vs. processing=post in static instances?

[twardoch]

Though I can see the benefit of processing=pre vs. post. But "no more". I mean, let's try to attack FEA in feaLib for more sophisticated control.

[twardoch]

Is it not sufficient to have an early lookup for the pre rules, and the very last GSUB lookup for the post rules, and have them both associated with "rvrn"? Or do implementations process all the lookups associated with rvrn early, regardless of their place in the lookup order?

[twardoch]

I mean, if I do

lookup ab { sub a by b; } ab;
lookup bc { sub b by c; } bc; 
lookup cd { sub c by d; } cd;

feature smcp { 
  lookup ab; lookup cd; 
} smcp;

feature ss01 { 
  lookup bc; 
} ss01;

Then, when I enter a and turn on both smcp and ss01, I’ll get d.

[justvanrossum]

Is it not sufficient to ...

No, or this PR would not exist. Check the 'rvrn' spec, and the fonttools issues related to this issue, a recent twitter discussion mentioned above, and a discussion on the OT list last year.

[justvanrossum]

Are we sure that extending the rules designspace section possibly ad infinitum is the best course of action?

In retrospect, I think it may have been enough for .designspace rules to only support "last" behavior. But it currently uses 'rvrn', which is pre per spec, and therefore pretty useless if interaction with other features is desired (which is often). And we can't just change the behavior silently, so the only way to address this is to add a flag to .designspace. I wish it hadn't been necessary.

[justvanrossum]

Then, when I enter a and turn on both smcp and ss01, I’ll get d.

Please read up on the actual issue before pointing out the obvious, which obviously doesn't work or we wouldn't be in this discussion.

[justvanrossum]

I think that "rules" should remain a simple hack

It's not a hack, just like *.ufo/kerning.plist is not a hack. It's a higher level description that is useful for the common case. Like kerning.plist, .designspace rules could be compiled via .fea syntax, if it existed. But it doesn't, so we made our own compiler.

Just like kerning.plist could be directly compiled to GPOS. I don't think that going through .fea is fundamentally the best option. It's convenient because it's easier to compile to .fea than to GPOS directly.

[twardoch]

Thanks for the clarification. Sorry for my ignorance :)

Of course I'm not opposing higher-level structures, but we know that rules predate FeatureVariations and were intended for swapping in instances.

My concern now is that implementing the pre vs post rules behavior in static instances is not specified at all, and may be quite hard to implement.

Unless the recommendation would be that post rules are also done in rclt in instances, just without conditions. Pre can best be done in cmap and ccmp, I think.

[twardoch]

With kerning vs. feature kern, it's slightly different. Kerning is deliberately a higher-level structure. Simple rules probably as well. But pre vs. post feels a bit like a hack, made to address the fact that the general-purpose feature building is not up to the task.

I know that adding it to rules addresses an immediate need — I just meant to say that it just gives a small step. For things like conditional turning on ligatures as you increase a variable axis value, it still doesn't work. So we still need to work on FEA, I think.

[justvanrossum]

For things like conditional turning on ligatures as you increase a variable axis value, it still doesn't work.

No, this is deliberately out of scope for designspace rules. Just like eg. script/language-specific kerning or contextual kerning is out of scope for kerning.plist.

So we still need to work on FEA, I think.

Sure! I'm all for it.

However, in the plus-three-years that people have been talking about this, I haven't seen any progress on the implementation side. And people still can't seem to agree how things should be spelled. And if it ever gets so far — mark my words — it will feel a lot more like a hack than designspace rules ever did.

[anthrotype]

Thanks for adding the test! LGTM