Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'clig' not required in 'khmr' #1310

Closed
bungeman opened this issue Oct 24, 2018 · 20 comments
Closed

'clig' not required in 'khmr' #1310

bungeman opened this issue Oct 24, 2018 · 20 comments

Comments

@bungeman
Copy link
Collaborator

No description provided.

@bungeman bungeman changed the title 'clig' not required in ' 'clig' not required in 'khmr' Oct 24, 2018
@bungeman
Copy link
Collaborator Author

There is a w3c issue at w3c/csswg-drafts#2644 which grew out of https://bugzilla.mozilla.org/show_bug.cgi?id=1458053#c4 which I believe mostly boils down to (at least) Noto Serif Khmer having required ligatures in the 'clig' feature and HarfBuzz not applying them when the user requests 'clig' off. Looking at https://docs.microsoft.com/en-us/typography/script-development/khmer#features it appears that 'clig' should be in 'khmer_features' instead of override-ably set in 'collect_features_khmer'.

@behdad
Copy link
Member

behdad commented Oct 24, 2018

In HarfBuzz we don't make a distinction between required and optional features. Every feature can be disabled by user, possibly resulting in broken rendering. That's by design. "Don't do it" is good enough answer for me. font-feature-settings is a low-level mechanism. cc @jfkthame @drott

I haven't read the thread.

@bungeman
Copy link
Collaborator Author

I see, thanks for the clarification. In this case though this isn't a result of font-feature-settings but one of letter spacing properties. The idea is to disable any optional ligatures when doing letter spacing and normally 'clig' is optional, but apparently not with 'khmr'. I assume in this case the user should collect the set of features enabled by default with a given language/script and then not disable, say 'clig', if it shows up as a default enabled feature? Otherwise it seems difficult for the user to keep up with the set of features which shouldn't be disabled.

@bungeman
Copy link
Collaborator Author

Actually, I don't think looking up the set of features enabled by default with a given language/script will work in this case, since hb_ot_shape_collect_features adds both 'planner->shaper->collect_features' and adds 'horizontal_features' (which contains 'clig') so I'm not sure the user can get this information. So it is on by default but needs to be off when letter spacing, except when we don't want to turn it off because the planner wanted it on.

@behdad
Copy link
Member

behdad commented Oct 24, 2018

Okay I see the problem. Looks like Khmer spec is abusing this feature indeed.

Don't know. I can possibly make certain features required. @jfkthame what do you think? I personally like being able to disable everything for testing, etc.

@bungeman
Copy link
Collaborator Author

I also dislike the idea of changing things so that 'clig' is just always blindly applied to 'khmr' with no way to disable it. Perhaps if there was some way for the user of HarfBuzz to get back just the set of 'strongly suggested' features so they could avoid changing / specifying / setting such features for non-explicit reasons.

@behdad
Copy link
Member

behdad commented Oct 24, 2018

I don't like exposing internals too much...

@behdad
Copy link
Member

behdad commented Oct 24, 2018

I think I'm fine forcing clig on in Khmer only and leave the rest of the system as is...

@bungeman
Copy link
Collaborator Author

There is currently one 'required' feature which can't be disabled by the user (as far as I know), and that is any feature referenced by the requiredFeatureIndex. In theory if Noto Khmer had pointed its 'khmr' requiredFeatureIndex at the 'clig' feature then it would 'just work'. Of course that doesn't help any other fonts which expect 'clig' to be non-disable-able in 'khmr'.

@khaledhosny
Copy link
Collaborator

Any feature can be disabled, even required features like rlig. I feel we are trying to fix the problem at the wrong level. If a feature shouldn’t be disabled for a certain script, then the client that is requesting it to be disabled shouldn’t do that in the first place. IMHO, keeping HarfBuzz flexible is a good feature to keep. requiredFeatureIndex is a mechanism specially designed to not allow any outside control over it, so it is fine to keep it this way.

@bungeman
Copy link
Collaborator Author

I think most users would probably be fine with adding a bit of extra code to 'filter' their feature requests, but it seems too great a burden for every user to know which features are marked as 'required' for all the scripts. Even if HarfBuzz simply carried the list of 'strongly suggested' ('Required') features per script as spec-ed by OpenType and allowed filtering against such a list that might be enough. It seems somewhat unfortunate to do it this way, since every user would have this extra step to perform to be 'correct'. On the other hand, only the user can always know the right list to filter. For css resolution this would be done to the candidate feature list just before applying font-feature-settings (and after letter-spacing resolution).

Alternatively, if there were some way to mark feature enable/disable requests as 'weak' or 'strong' that may also be a viable option, though I'm not sure how to specify that or how it would be backwards compatible.

@behdad
Copy link
Member

behdad commented Oct 25, 2018

Okay I have a satisfactory solution for this now.

Shapers can override features enabled by the common shaper body, in their override_features method. Currently we have these overrides:

  • hangul:
  /* Uniscribe does not apply 'calt' for Hangul, and certain fonts 
   * (Noto Sans CJK, Source Sans Han, etc) apply all of jamo lookups 
   * in calt, which is not desirable. */ 
  plan->map.disable_feature (HB_TAG('c','a','l','t')); 
  • indic:
  plan->map.disable_feature (HB_TAG('l','i','g','a')); 
  • khmer:
  /* Uniscribe does not apply 'kern' in Khmer. */ 
  if (hb_options ().uniscribe_bug_compatible) 
  { 
    plan->map.disable_feature (HB_TAG('k','e','r','n')); 
  } 
  
  plan->map.disable_feature (HB_TAG('l','i','g','a')); 
  • myanmar:
  plan->map.disable_feature (HB_TAG('l','i','g','a')); 

Note how this disable features that are otherwise advertised as enabled. This causes a problem now. If user actively explicitly these features, user's wish overrides the shaper's override! That's not desired. So, I'm going to suggest that we make override_features later to override user's requests as well. At that point, we just need to add clig to khmer override_features.

@jfkthame ?

@jfkthame
Copy link
Collaborator

I'm going to suggest that we make override_features later to override user's requests as well.

Not sure I agree with this. ISTM that explicit user requests should override what the shaper does automatically -- whether that's applying "common" features like 'locl' or 'kern', or script-specific shaping like 'init', etc. If I'm shaping Arabic, and explicitly pass init=0, I expect harfbuzz to do its standard Arabic shaping but to leave the 'init' feature globally disabled. Sure, the result is broken text, but that's what I asked for.

Likewise, if I'm shaping Khmer and pass clig=0, I think that should override what the shaper normally does. I'd argue that Khmer fonts that use clig for "required" ligatures that should be used even in the presence of letter-spacing are bad fonts; they should be using rlig for those. Nevertheless, a user can still work around this if desired, by explicitly setting 'clig' on, thus overriding the automatic disablement of ligatures due to letter-spacing.

@behdad
Copy link
Member

behdad commented Oct 25, 2018

I'd argue that Khmer fonts that use clig for "required" ligatures that should be used even in the presence of letter-spacing are bad fonts;

That's a Khmer spec bug more than a font bug.

I generally agree with you. The overrides are specific things that need intervention. Currently, if user / browser explicitly asks for +liga=1 that breaks Khmer shaping with many fonts. That's not good. My proposal fixes that.

So, the aim of the overrides, is to make liga, kern, and clig be user-controlable features in effect, and not break shaping.

@behdad
Copy link
Member

behdad commented Nov 1, 2018

@jfkthame can we agree on this?

@bungeman
Copy link
Collaborator Author

It may be a silly idea, but is it possible to have a special feature (like 'HARF' and 'BUZZ') control this? Say there were a feature 'RQRD'

on - required features non-optional (always on no matter what, never break shaping).
off - skip setting any script required features (just skip setting them for testing, demos, etc).
unset - set required features but allow them to be overridden (what is currently done).

Note that this falls a bit short of the ideal ability to filter individual feature requests (have a set of 'strong' requests and 'weak' requests, filtering the 'weak' requests against the script required features). Instead, this allows for all feature requests to be made 'weak' (RQRD on), 'strong' (RQRD unset), or 'very strong' (RQRD off). However, it would preserve the existing behavior but allow users to opt into either full control or never-break-shaping behavior. As a vendor feature request this is also ignore-able, so should be at least somewhat compatible.

I'm not saying this is a great idea, but maybe something like this could be made to work?

@behdad
Copy link
Member

behdad commented Nov 29, 2018

I don't see how that would fix the problem at hand.

@bungeman
Copy link
Collaborator Author

It would allow a user to set the 'RQRD' feature to change the behavior to 'never break shaping' (so 'khmr' can turn on 'clig' and no user feature request will turn it back off), making HarfBuzz act more like other shaping engines with respect to feature requests. However, it also allows the user to request the existing 'break shaping if you have to' behavior as #1310 (comment) requests. It also provides the test feature of 'ignore all the script features' used by the test framework. These three use cases seem to be the most common (the ones I know about).

Obviously this could be done in some other way than providing a meta-feature, but that just occurred to me as a simple backward compatible way to introduce this. I don't think it's the most ideal, but maybe something like this could make opting into 'never break shaping' an acceptable setting.

@behdad
Copy link
Member

behdad commented Nov 29, 2018

By user you mean client code here I assume. Whatever we do, web browsers would need to pick one behavior as default. We should simply make that be the default and only supported mode. Providing more options doesn't make sense to me.

@behdad
Copy link
Member

behdad commented Nov 29, 2018

simple backward compatible way to introduce this

Backward compatibility is not a concern in this case, as we long as we are bug fixing and not changing API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants